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

Use NcSelect for NcTimezonePicker #3781

Merged
merged 2 commits into from
Feb 21, 2023
Merged

Conversation

raimund-schluessler
Copy link
Contributor

@raimund-schluessler raimund-schluessler commented Feb 17, 2023

This PR moves NcTimezonePicker from using NcMultiselect to NcSelect. Since NcSelect does not support grouping, I had to work around this a bit, but the outcome is the same as when using NcMultiselect. 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.

Before After
image image

Fixes 1/3 of #3743.

@raimund-schluessler raimund-schluessler added 3. to review Waiting for reviews feature: select Related to the NcSelect* components technical debt labels Feb 17, 2023
@raimund-schluessler raimund-schluessler changed the title Use NcSelect for timezone picker Use NcSelect for NcTimezonePicker Feb 20, 2023
Copy link
Contributor

@jancborchardt jancborchardt left a 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. :)

@raimund-schluessler raimund-schluessler mentioned this pull request Feb 20, 2023
3 tasks
@raimund-schluessler
Copy link
Contributor Author

  • 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 NcSelect component where theses styles are coming from. See here a plain NcSelect component, which shares the same inconveniences:

NcSelect

So I would rather not touch this here and make it a separate issue/PR, see #3798.

Copy link
Contributor

@jancborchardt jancborchardt left a 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]>
@raimund-schluessler raimund-schluessler marked this pull request as draft February 21, 2023 19:46
@raimund-schluessler
Copy link
Contributor Author

I adjusted the search code a bit, so that the continent labels are shown if one region matches, just like with the old picker.

@raimund-schluessler raimund-schluessler marked this pull request as ready for review February 21, 2023 19:54
@raimund-schluessler raimund-schluessler added the enhancement New feature or request label Feb 21, 2023
@raimund-schluessler raimund-schluessler merged commit f96d606 into master Feb 21, 2023
@raimund-schluessler raimund-schluessler deleted the fix/3743/timezone-select branch February 21, 2023 20:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3. to review Waiting for reviews enhancement New feature or request feature: select Related to the NcSelect* components technical debt
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants