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

fix: removed groupings in timezone picker #5130

Conversation

emoral435
Copy link
Contributor

☑️ For nextcloud/server#41842

🖼️ Screenshots

🏚️ Before 🏡 After
firefox_zBqhJRiF04 firefox_9PTdGieRfq

🚧 Tasks

  • Removed the groupings from the TimeZone picker. This does two things:
  1. Removes the styling issue presented in the before picture (contrast ratio for the heading of the grouping not being correct. This is due to NcSelect marking the header as a non-selectable element due to us passing in the header as an object)
  2. Now leaves us (for later!) to decide how to better organize the groupings better visually, as right now, I personally do not like the presentation of the areas. But that is me, personally, and we can always work on a better design later :)

🏁 Checklist

  • ⛑️ Tests are included or are not applicable
  • 📘 Component documentation has been extended, updated or is not applicable

@emoral435 emoral435 added bug Something isn't working 3. to review Waiting for reviews accessibility Making sure we design for the widest range of people possible, including those who have disabilities labels Jan 24, 2024
@emoral435 emoral435 added this to the 8.6.0 milestone Jan 24, 2024
@emoral435 emoral435 self-assigned this Jan 24, 2024
@emoral435 emoral435 force-pushed the fix/a11y/41842/NcTimezonePicker-group-headings-do-not-meet-contrast-ratio branch from c00d9be to 4c89169 Compare January 24, 2024 17:11
@emoral435
Copy link
Contributor Author

Also - would love to have the opinion from all the reviewers.

Would you personally leave the commented out code? This is a library, and so I am leaning on taking out the commented code (it was what was causing the problem in the first place), but due to the fact that we may later group the areas again, it MAY be used fr the logic of grouping areas (probably not, though, as all it does it create an object that NcSelect doesnt let you click on)

Signed-off-by: Eduardo Morales <emoral435@gmail.com>
@Pytal
Copy link
Contributor

Pytal commented Jan 25, 2024

Would you personally leave the commented out code?

In general I would remove it, otherwise we'd have a lot of commented code laying around just in case

Copy link
Contributor

@Pytal Pytal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Other than removing the commented code

@ShGKme
Copy link
Contributor

ShGKme commented Jan 25, 2024

In general I would remove it, otherwise we'd have a lot of commented code laying around just in case

+1 here. We always can re-take the code from history when it is needed

Copy link
Contributor

@ShGKme ShGKme left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sad, not we have no other easy solution with NcSelect for now

@susnux
Copy link
Contributor

susnux commented Jan 25, 2024

+1 here. We always can re-take the code from history when it is needed

Thats why we use a version control system 😉

@skjnldsv skjnldsv merged commit 7fdd4e0 into master Jan 25, 2024
15 checks passed
@skjnldsv skjnldsv deleted the fix/a11y/41842/NcTimezonePicker-group-headings-do-not-meet-contrast-ratio branch January 25, 2024 08:09
@raimund-schluessler
Copy link
Contributor

As I understand it, the problem was just about the constrast ratio, no? Why didn't we simply adjust the colors in the CSS of the picker? Or was there another issue that was not described?

@susnux susnux modified the milestones: 8.6.0, 8.5.1 Jan 25, 2024
@ShGKme
Copy link
Contributor

ShGKme commented Jan 25, 2024

As I understand it, the problem was just about the constrast ratio, no? Why didn't we simply adjust the colors in the CSS of the picker? Or was there another issue that was not described?

If I remember correctly, no.

The main issue is about semantics. We cannot have groups only visually, where group is only defined by disabled and designed as a header list item. Groups have to be really a group with a header inside.

cc @emoral435 @Pytal @JuliaKirschenheuter to clarify, if I'm wrong

@susnux susnux mentioned this pull request Jan 26, 2024
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 accessibility Making sure we design for the widest range of people possible, including those who have disabilities bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants