ui: Create PopoverSelect, CatalogToolbar, and update tests #7489
Conversation
kaxcode
force-pushed
the
ui/feature/create-popover-select
branch
3 times, most recently
from
748b49d
to
377201a
Compare
There was a problem hiding this comment.
Amazing 👏
I left one little nitpick comment, but it can always wait for another PR.
Oh actually, I just gave it a quick try and the sort menu should probably close when you change the sort. Have a look at #7479 you should see how to do it (that PR was essentially the same problem). Shout me if you can't figure it though.
My other thought should we make the sort button trigger the same height as the search box? And maybe see if we can move the popover-panel up a few pixels so its exactly underneath the trigger button when its open. I was just looking at the designs for this and those two things would make it spot on!
| margin-top: 8px; | ||
| width: 16px; | ||
| height: 16px; | ||
| } |
There was a problem hiding this comment.
Wondering if there's some stuff that could go in skin.scss here, maybe from line 57 down as that is all to do with the pipe and the icon, don't worry too much though we can always hit that in another spring clean PR, or decide when we next chat, its no biggie.
Oh wow I just saw what you did with the 103%, I'm guessing this is to do with making sure the top and bottom of the pipe meet up with the borders? I love the 103% btw, but I just has a thought that maybe a 'calc(100% + 3px)' might be better (if thats what this is for of course?)
There was a problem hiding this comment.
I'll move those lines over to skin.scss.
calc(100% + 3px) is not the same as 103% so it extends too far down. The exact px match would be calc(100% + .5px). Which I think looks worse than the 103%. 🤔
There was a problem hiding this comment.
Update on the height. I had the sort matching the search before but I must have undone the change. Adding this change removes the need of a height: 103%.
There was a problem hiding this comment.
@johncowen The changes in PR #7479 are at another level than the ones in PopoverSelect. I don't think I can work with the fake onchange event that was created to listen to an event listener, but we can talk about it more tomorrow.
kaxcode
force-pushed
the
ui/feature/create-popover-select
branch
from
377201a
to
8458aad
Compare
kaxcode
force-pushed
the
ui/feature/create-popover-select
branch
from
8458aad
to
1187a36
Compare
johncowen
force-pushed
the
ui-staging
branch
2 times, most recently
from
dc39913
to
cc0247c
Compare
kaxcode
force-pushed
the
ui/feature/create-popover-select
branch
2 times, most recently
from
3a0a724
to
00a518f
Compare
kaxcode
force-pushed
the
ui/feature/create-popover-select
branch
2 times, most recently
from
ff1c139
to
a188949
Compare
kaxcode
force-pushed
the
ui/feature/create-popover-select
branch
2 times, most recently
from
b9a1cf2
to
ae4e94a
Compare
Preferably we want all copy/text to live in the template. Whilst you can achieve what we've done here with a combination of different helpers, as we will be using this approach in various places it's probably best to make a helper. We also hit an ember bug related to using the `let` helper and trying to access `thingThatWasLet.firstObject` (which can also be worked around using `object-at`). Moving everything to a helper 'sorted' everything. Probably worthwhile noting that if the sort option themselves become dynamic, I'm not sure if the helper here would actually react as you would expect (I'm aware that ember helpers on react on the root arguments, not necesarily sub properties of those arguments). If we get to that point this helper could take the same approach as what I believe ember-composable-helpers does to get around this, or move them to the view controller. If we do ever moved this to the view controller, we can still use the exported function from the new helper here to keep using the same functionality and tests we have here.
kaxcode
force-pushed
the
ui/feature/create-popover-select
branch
from
ae4e94a
to
a55ccb2
Compare
|
Future PR needed to improve the performance by having the the sorting happen before the filtering. |
I love that we have this new dropdown menu component to use elsewhere ❤️* Create PopoverSelect component and styling * Create CatalogToolbar component and Styling * ui: Adds `selectable-key-values` helper (#7472) Preferably we want all copy/text to live in the template. Whilst you can achieve what we've done here with a combination of different helpers, as we will be using this approach in various places it's probably best to make a helper. We also hit an ember bug related to using the `let` helper and trying to access `thingThatWasLet.firstObject` (which can also be worked around using `object-at`). Moving everything to a helper 'sorted' everything. Probably worthwhile noting that if the sort option themselves become dynamic, I'm not sure if the helper here would actually react as you would expect (I'm aware that ember helpers on react on the root arguments, not necesarily sub properties of those arguments). If we get to that point this helper could take the same approach as what I believe ember-composable-helpers does to get around this, or move them to the view controller. If we do ever moved this to the view controller, we can still use the exported function from the new helper here to keep using the same functionality and tests we have here. * Create tests for sorting services with CatalogToolbar * Add rule to print 'ember/no-global-jquery' as a warning Co-authored-by: John Cowen <johncowen@users.noreply.github.com>
* Create PopoverSelect component and styling * Create CatalogToolbar component and Styling * ui: Adds `selectable-key-values` helper (#7472) Preferably we want all copy/text to live in the template. Whilst you can achieve what we've done here with a combination of different helpers, as we will be using this approach in various places it's probably best to make a helper. We also hit an ember bug related to using the `let` helper and trying to access `thingThatWasLet.firstObject` (which can also be worked around using `object-at`). Moving everything to a helper 'sorted' everything. Probably worthwhile noting that if the sort option themselves become dynamic, I'm not sure if the helper here would actually react as you would expect (I'm aware that ember helpers on react on the root arguments, not necesarily sub properties of those arguments). If we get to that point this helper could take the same approach as what I believe ember-composable-helpers does to get around this, or move them to the view controller. If we do ever moved this to the view controller, we can still use the exported function from the new helper here to keep using the same functionality and tests we have here. * Create tests for sorting services with CatalogToolbar * Add rule to print 'ember/no-global-jquery' as a warning Co-authored-by: John Cowen <johncowen@users.noreply.github.com>
selectable-key-valueshelper (ui: Addsselectable-key-valueshelper #7472)co-authored by: @johncowen