-
Notifications
You must be signed in to change notification settings - Fork 13
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
Multiselect: Make arrow open/close drop-down #1180
Conversation
Thanks for the improvements! Browse a preview of your changes using the link below. Built with commit dd16324 |
Thanks for the improvements! Browse a preview of your changes using the link below. Built with commit 3143a3b |
looks good! |
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.
Works as advertised, but I do have one question.
140 = the max-height value set in multiselect.less for the fieldset. | ||
*/ | ||
if ( event.layerX > target.offsetWidth - 35 && | ||
_fieldsetDom.offsetHeight === 140 ) { |
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.
Is this the best way we have to check that the fieldset is open?
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.
Yeah I know it's hackery :( I think we need to do some re-writing of the drop-down. It opens when the search input gets focus and closes when it's blurred. Focus happens on mousedown, so it gets the attributes of being open when it is clicked, not when the drop-down has actually completed opening. Getting the height was the only way I found to reliably find that it was actually open. We should re-write this to dispatch events for the life cycle of the drop-down expand/collapse and then listen to the expandEnd
event (or actually probably the transitionEnd
event) and add attributes at that point, which this if statement would then query. As noted in this PR, the collapse method is what dispatches (erroneously I think) the expandEnd
event.
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.
Makes sense. This is fine for now, but could you capture everything you've said here in a new issue? (Or do we already have one open?)
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.
35 = width of the arrow on the right of the search input. | ||
140 = the max-height value set in multiselect.less for the fieldset. | ||
*/ | ||
if ( event.layerX > target.offsetWidth - 35 && |
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 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.
Yes, good catch. It would be nice to find a better way to target the dropdown in the future…
When you open the multiselect, let's make it so the down-arrow on the right-hand side closes it when clicked, yaaay! Also, let's flip the arrow when it is open! Yaaaah!
Changes
Testing
Screenshots