-
-
Notifications
You must be signed in to change notification settings - Fork 467
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Replace uses of "dropdown" with respective ViewComponent #5907
base: main
Are you sure you want to change the base?
Replace uses of "dropdown" with respective ViewComponent #5907
Conversation
Only one dropdown button were using three dots icons hence it is better to put it on block of component from html.erb.
display: flex and align-items: center added to style to align items of button horizontally. button_class variable added to dropdown component to dynamically change css properties of button like background color
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks pretty solid overall but I think the actually using the dropdown component exposes some of the issues with it. I think we can improve the component a bit.
The replacements themselves look good to me though.
@@ -1,12 +1,7 @@ | |||
<div class="dropdown <%= @class %>"> | |||
<button class="btn btn-secondary dropdown-toggle" type="button" data-bs-toggle="dropdown" aria-expanded="false"> | |||
<button class="btn btn-secondary dropdown-toggle <%= @button_class %>" style="display: flex; align-items: center;" type="button" data-bs-toggle="dropdown" aria-expanded="false"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can this be done with bootstrap styles? d-flex
and align-items-center
? I would prefer to avoid combining styling via classes and via style attributes.
<% else %> | ||
<svg width="5" height="20" viewBox="0 0 5 20" fill="none" xmlns="http://www.w3.org/2000/svg"> | ||
<title><%= @menu_title %></title> | ||
<path d="M2.5 0.9375C1.82292 0.9375 1.23698 1.17188 0.742187 1.64063C0.247396 2.10938 -7.68364e-08 2.69531 -1.07571e-07 3.39844C-1.38306e-07 4.10156 0.247396 4.70052 0.742187 5.19531C1.23698 5.6901 1.82292 5.9375 2.5 5.9375C3.17708 5.9375 3.76302 5.6901 4.25781 5.19531C4.7526 4.70052 5 4.10156 5 3.39844C5 2.69531 4.7526 2.10938 4.25781 1.64063C3.76302 1.17188 3.17708 0.9375 2.5 0.9375ZM2.5 7.5C1.82292 7.5 1.23698 7.7474 0.742187 8.24219C0.247395 8.73698 -3.66538e-07 9.32292 -3.96134e-07 10C-4.25731e-07 10.6771 0.247395 11.263 0.742187 11.7578C1.23698 12.2526 1.82292 12.5 2.5 12.5C3.17708 12.5 3.76302 12.2526 4.25781 11.7578C4.7526 11.263 5 10.6771 5 10C5 9.32292 4.7526 8.73698 4.25781 8.24219C3.76302 7.7474 3.17708 7.5 2.5 7.5ZM2.5 14.0625C1.82292 14.0625 1.23698 14.3099 0.742187 14.8047C0.247395 15.2995 -6.53963e-07 15.8984 -6.84698e-07 16.6016C-7.15432e-07 17.3047 0.247395 17.8906 0.742187 18.3594C1.23698 18.8281 1.82292 19.0625 2.5 19.0625C3.17708 19.0625 3.76302 18.8281 4.25781 18.3594C4.7526 17.8906 5 17.3047 5 16.6016C5 15.8984 4.7526 15.2995 4.25781 14.8047C3.76302 14.3099 3.17708 14.0625 2.5 14.0625Z" fill="#5D657B" /> | ||
</svg> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What was the logic for moving this out?
The three dots is a bit of a weird default but I believe we should have a default icon so it is more in line with most of our buttons (such as a chevron-down
)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There were two reasons of it.
- There are some dropdowns which are not using any icons so having default icons making no sense in that case. eg filters dropdowns in supervisor#index volunteers#index
- Only one dropdown button was using this three dots icons.
If keeping this as default, we have to add one more variable in component to not render icons for 1st case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That makes sense. Moving it out is a good idea, I didn't realize that dropdown-toggle
adds a ::after
element that is a chevron by default.
</div> | ||
<% end %> | ||
<%= render(DropdownMenuComponent.new(menu_title: "Assigned to Volunteer", klass: "pull-left mx-2 my-1 assigned-to-volunteer-options")) do %> | ||
<div class="dropdown-item form-check checkbox-style"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All the outer elements within the dropdowns should be li
instead of div
.
@class = klass | ||
@button_class = button_klass |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
button_class
makes sense. Lets take that 1 step further and lets make klass
container_class
</div> | ||
<% end %> | ||
<%= render(DropdownMenuComponent.new(menu_title: "Supervisor", klass: "pull-left mx-2 my-1 supervisor-options")) do %> | ||
<div class="dropdown-item form-check checkbox-style"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
would it make sense to make the dropdown items into components? Just have them be a
<li class="dropdown-item <%= @class_vars %>"></li>
that held whatever content you gave it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It can work out in separate component but not in this menu dropdown component.
@@ -26,6 +27,6 @@ def render? | |||
end | |||
|
|||
def button_label | |||
content_tag(:span, @menu_title, class: @hide_label ? "sr-only" : nil) | |||
content_tag(:span, @menu_title, class: @hide_label ? "sr-only" : "mr-5") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
content_tag(:span, @menu_title, class: @hide_label ? "sr-only" : "mr-5") | |
content_tag(:span, @menu_title, class: ["mr-5", {"sr-only" => @hide_label}]) |
could do this via https://edgeapi.rubyonrails.org/classes/ActionView/Helpers/TagHelper.html#method-i-token_list
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey thanks for the work. This is super close but just minor find and replace text changes.
Since the dropdown menu wraps its contents in a ul
the elements ought to be li
@@ -3,12 +3,13 @@ | |||
class DropdownMenuComponent < ViewComponent::Base | |||
renders_one :icon | |||
|
|||
def initialize(menu_title:, icon_name: nil, hide_label: false, render_check: true, klass: nil) | |||
def initialize(menu_title:, icon_name: nil, hide_label: false, render_check: true, container_klass: nil, button_klass: nil) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
def initialize(menu_title:, icon_name: nil, hide_label: false, render_check: true, container_klass: nil, button_klass: nil) | |
def initialize(menu_title:, icon_name: nil, hide_label: false, render_check: true, container_class: nil, button_class: nil) |
super minor nit
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These div
wrappers should be li
for accessibility reasons.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same with these div
s
What github issue is this PR for, if any?
Resolves #5845
What changed, and why?
How is this tested? (please write tests!) 💖💪
Note: if you see a flake in your test build in github actions, please post in slack #casa "Flaky test: " :) 💪
Note: We love capybara tests! If you are writing both haml/js and ruby, please try to test your work with tests at every level including system tests like https://github.com/rubyforgood/casa/tree/main/spec/system
Screenshots please :)
Run your local server and take a screenshot of your work! Try to include the URL of the page as well as the contents of the page.
Feelings gif (optional)
What gif best describes your feeling working on this issue? https://giphy.com/
How to embed:
![alt text](https://media.giphy.com/media/1nP7ThJFes5pgXKUNf/giphy.gif)