Skip to content
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

Add is-open class to the button when dropdown is open #48

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

doubleedesign
Copy link

Enables styling for the button specific to the open state, such as rotating the arrow.

Enables styling for the button specific to the open state, such as rotating the arrow.
@doubleedesign doubleedesign force-pushed the master branch 2 times, most recently from b3f79b0 to 3d432d2 Compare July 22, 2022 12:05
@zoltantothcom
Copy link
Owner

@doubleedesign Hey Leesa, just wanted to ask what do you think about having the same openClass for both the list and button? What's better in your opinion:

  1. Let them define ul.openClass and button.openClass in the CSS and call it a day, or
  2. Create a new class for buttons, i.e. openClassButton and rename the existing to openClassList, so we have two distinct classes each for its own element?

@doubleedesign
Copy link
Author

Thanks for the quick response @zoltantothcom!

I think both have their merits: I did #1 for simplicity, to not make more changes to the existing code than is necessary to achieve this feature, but your second suggestion is potentially more robust - there would be no confusion about what .is-open is referring to in the CSS if it isn't targeted more specifically. I'm not sure what the best thing to call the button class would be though - maybe something like list-is-open or dropdown-is-open.

@zoltantothcom
Copy link
Owner

My first thought was about renaming .is-open --> .is-open-list and having the new .is-open-button, but that can potentially break it for people who already have styles for .is-open, so it's probably better to leave it as is. And the new one - for consistency with .is-selected and .is-open that's already there, I'd name it .is-open-btn or something like that 🤷‍♂️

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants