UI: Multi-select dropdown component #5200
Conversation
| }, | ||
|
|
||
| openOnArrowDown(dropdown, e) { | ||
| this.capture(dropdown); |
There was a problem hiding this comment.
If the capture method only does a set, would it be best to do the set here? Do you plan on having any other code in the capture method?
There was a problem hiding this comment.
There's no other code in capture, but capture is also called in the template in the onOpen hook.
There was a problem hiding this comment.
Ah gotcha, missed that, makes sense then 👍
| this.capture(dropdown); | ||
|
|
||
| if (!this.get('isOpen') && e.keyCode === ARROW_DOWN) { | ||
| dropdown.actions.open(e); |
There was a problem hiding this comment.
Not sure, but you may need to bind this to dropdown so this within actions.open refers to the component rather than the actions object - guessing here btw. If you do need to do that, don't miss the close one below also.
There was a problem hiding this comment.
It works as is and this is how it is used in the docs.
There was a problem hiding this comment.
Cool, if thats how they document it then fair enough
There was a problem hiding this comment.
Sorry! I've had this mid review for most of the day and forgot it was there. Just a couple of additions, nothing major. Like the freestyle demo page btw! I spotted the more or less the same visual thing as alisdair. Although I saw it on other menus apart from the right hand one, shout me if you need any extra detail.
| dropdown.actions.close(e); | ||
| // Return focus to the trigger so tab works as expected | ||
| const trigger = this.element.querySelector('.dropdown-trigger'); | ||
| if (trigger) trigger.focus(); |
There was a problem hiding this comment.
Below here where you preventDefault it's wrapped within the conditional - this is actually different, but thought I'd check in case the preventDefault here should be inside the conditional also?
There was a problem hiding this comment.
Hm. It probably should, just for the correctness, but in practice I'm not concerned about the ESC key. The reason to be careful about preventDefault in the UP and DOWN case is preventDefault prevents page scrolling with arrow keys, so I wanted to make sure if you mash the down key it will traverse the full list, then carry on scrolling the page.
There was a problem hiding this comment.
Cool no prob, I see why its different, thanks!
ui/app/helpers/includes.js
Outdated
| return haystack.includes(needle); | ||
| } | ||
|
|
||
| export default Helper.helper(includes); |
There was a problem hiding this comment.
It looks like ember-composable-helpers contains does the same as this? I might be wrong as it looks like it does a bit more.
There was a problem hiding this comment.
D'oh, I should definitely use contains instead of my own thing. Will change.
|
@alisdair and @johncowen, thanks for the reviews! I looked more into the positioning bug and learned a couple things.
I suspect this has to do with some weird CSS relatively positioned ancestors headache, but I'm not sure yet. I'll give it a couple hours to see if I can't fix it before merging. |
|
@alisdair and @johncowen, I pushed another commit to fix the wonky top positioning issue. It had to with dimension calculations happening before the dropdown was moved into the wormhole. I fixed it with css, but I also filed an issue to fix the underlying issue of prematurely measuring dimensions: cibernox/ember-basic-dropdown#443 |
|
I'm going to lock this pull request because it has been closed for 120 days ⏳. This helps our maintainers find and focus on the active contributions. |
This is part one of three for faceted search in the Nomad UI
This is a relatively straightforward component for selecting multiple attributes in a single group.
It takes a set of options, a selection set, and an
onSelecthandler as inputs. With this it constructs a dropdown list.It's unfortunately at a weird midpoint between
ember-basic-dropdownandember-power-select, meaning it's built on basic dropdown, but I needed to reimplement some of the nice user expectations that power select already has. This includes things like keyboard navigation and focus management.In this last screenshot you can start to see where I'm going with this. Parts two and three of this project will be utilizing this component on the Jobs and Clients page to introduce faceted search/filtering by attribute.