fix: enable HC switcher, add beta label #4766
Conversation
There was a problem hiding this comment.
- For the heading level, Austin's original PR I think had it as an h1. I'm not sure if any heading level will be great - h1 is slightly better because then it doesn't come across as a sub-section of some other h1 on the page (on component pages for example, "High contrast" comes off as a sub-section of the component's CSS variables heading). An h2 might work if there were visible text above the dark/system/light modes area since that renders an h1, just right now an empty h1 is being rendered because we don't pass a label in. Visually, though, we've had these menu groups with headings/labels act as independent groups rather than "This is the label/heading for the entire menu, and this label/heading is a sub-section".
- For the function bit, I think the React implementation isn't actually setup properly to accept a function? It's typed as only a React Node, but even then we aren't rendering it correctly if a React Node is passed in since the check to wrap stuff in that title class only accepts a function or string. Do we recall the reason for only wrapping the label in this menu groujp class in this way? Passing a Title in doesn't provide the same styling as just a string so either that can't be it or we missed updating some CSS to take that into account.
|
My 2 cents: what if we add a help popover beside the beta label that either links to Medium article or incoming Theming docs? Like this: 
Maybe with something like this copy:
I think it's clear enough without it, but would help link to additional context more conveniently 🤷♀️ |
I think a label would be good! How about "Display Mode" or "Color Mode" |
|
Use "Color scheme" and "High contrast" for group titles |
Deploying patternfly-org with
|
| Latest commit: |
b11ea73
|
| Status: | 🚫 Build failed. |
@thatblindgeye did a little diggin', does this help? Issue: patternfly/patternfly-react#8209 And here's a PR where they used it in product - RedHatInsights/frontend-components#1628 |
There was a problem hiding this comment.
File comment below will depend on this comment for conversation:
I'm wondering if we should allow div and span to be passed as the "title/heading" elements for MenuGroup. Because those are all being appended to the document body, the heading levels are tough to land on which level they should be, or it may not end up making sense.
For example, the Popover heading level is an h3, but it doesn't have any h2 ancestors (nor h1), so it ends up being a random h3 within the page. This Popover may actually work as an h1 being passed as the heading element instead, because at least with VoiceOver, wwhen traversing the rotor menu that's the only heading that shows up since the Popover has the modal attribute. Can confirm in NVDA the Popover heading is also treated as the only heading accessible when trying to navigate with keyboard shortcuts to next/previous headings, so there is a case for this being an h1.
For the MenuGroups, however, their h2 level really only works on the main page of Org since they act as subsections of the h1 there, but on component pages they end up coming off as subsections of the component itself. Thinking about whether it would make more sense to make the current headings in the theme menu plain divs, and just link them to the Select groups via aria-labelledby so that the Select lists are properly named.
| <h2 className="pf-v6-c-menu__group-title"> | ||
| High contrast{' '} | ||
| <Popover |
There was a problem hiding this comment.
This may depend on discussion of my PR comment above, but if we do decide to keep heading levels within this menu, could we structure this in a way so that the popover button and beta label aren't part of the heading? This is what the heading ends up looking like in the a11y tree:
Which feels odd for some reason. I think if it was just the High Contrast text and the Beta label it'd look a little better.
| onOpenChangeKeys={['Escape']} | ||
| > | ||
| <SelectGroup> | ||
| <SelectGroup label={'Color scheme'} labelHeadingLevel="h2"> |
There was a problem hiding this comment.
Also depending if anything changes from above comment, but since we're giving a visible label here let's also update the aria-label of the SelectList to match this visible label.
|
@thatblindgeye thanks for the feedback! In this update 34e5308
Other changes unrelated to your feedback
|
fixes #4765
fixes #4759
Questions:
<SelectGroup label="...">and have it generate the menu title element/classes so I don't have to hard code it? According to the code, I assumed if I pass a function, it would include the function within<Wrapper />but it doesn't appear to.<h2>seemed appropriate considering it's a top-level heading for the page, not necessarily related to the content (cc @thatblindgeye)