-
Notifications
You must be signed in to change notification settings - Fork 93
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
Use NcSelect
for NcTimezonePicker
#3781
Conversation
NcSelect
for NcTimezonePicker
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.
Very cool! :) I think the blue border around it is actually nice.
3 things:
- Style-wise it coukd look a bit more like the ActionMenu component, with the entries not expanding the whole width, but having a bit of margin and having a birder-radius
- The contrast of the highlight seems to not be enough for accessibility, here we should also do it however we make the ActionMenu entry hover/focus accessible cc @JuliaKirschenheuter @Pytal
- Detail: The height of the co tainer should (if possible) always cut the list at ½ of the last entry, so that one entry is cut off and it is obvious to people that the list continues. :)
I think this is all valid feedback, but it affects the So I would rather not touch this here and make it a separate issue/PR, see #3798. |
68b4cf6
to
53fd453
Compare
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.
Sounds good, thanks for opening the issue, approved design-wise then. :)
Signed-off-by: Raimund Schlüßler <[email protected]>
53fd453
to
e0cca59
Compare
Signed-off-by: Raimund Schlüßler <[email protected]>
I adjusted the search code a bit, so that the continent labels are shown if one region matches, just like with the old picker. |
This PR moves
NcTimezonePicker
from usingNcMultiselect
toNcSelect
. SinceNcSelect
does not support grouping, I had to work around this a bit, but the outcome is the same as when usingNcMultiselect
. I additionally enhanced the search function a bit, so one can now also search for the continent name and see all timezones e.g. in Europe, and searching for "Europe London", will give the correct timezone.The styling probably needs a bit of adjustment still, because it currently uses the default
NcSelect
style with a gray/blue border, whereas the current timezone picker has no border. But maybe we want to update the style here and match with the other pickers, so I left it for now.Fixes 1/3 of #3743.