feat(table): [PF-3917] document tokens in all themes/modes#141
Conversation
|
|
||
| return ( | ||
| <Tooltip content={fullLabel} position="top"> | ||
| <span className="ws-theme-display-label ws-theme-abbr-trigger" tabIndex={0}> |
There was a problem hiding this comment.
This should be wrapping a plain button or other interactive element for accessibility.
| white-space: nowrap; | ||
| } | ||
|
|
||
| .ws-token-value-popover-trigger { |
There was a problem hiding this comment.
I think the popover triggers shouldn't need so much custom CSS for formatting if they are using plain Buttons
thatblindgeye
left a comment
There was a problem hiding this comment.
Some thoughts:
- Could we omit the theme info from the "Values" cell if "All themes" isn't selected? It would free up some space, and I'm not sure if we need to provide abbreviations if we're showing a token value for a single theme; the Select toggle should provide the context of the values shown in the table whenever "All themes" isn't selected
- We could maybe make a dynamic Table caption whenever "all themes" isn't selected - e.g. "Semantic tokens for Default theme, light mode, and default contrast"
- When "All themes" is selected and the table displays the various "X themes" text, I'm wondering if instead of a tooltip we could use a Popover, and within that render an unordered list of the themes. Something like this:
- Using a popover, we'd want to use a button on the trigger like Nicole said, and should also apply
aria-haspopup="dialog"to the button - Also, we'd want to update the popover when clicking the token value so that we use an unordered list for the themes listed there as well
- Using a popover, we'd want to use a button on the trigger like Nicole said, and should also apply
- Maybe could use some input from others regarding the verbiage, but what do we think about using "theme" for the default or unified, "mode" for the light or dark, and "contrast" for, well, the contrast? idk if we really ever landed on what verbiage to use and think we've kinda used "theme" a bit interchangeably
- Related, what do others think about replacing the vertical pipes in "Theme | Mode | Contrast" with "Theme, Mode, and Contrast"?
- May not necessarily need to be as part of this PR, but we need to update the derived token value element in the "Values" cell. Right now it's just a span that applies PF button classes; it needs to be an actual Button component with the
aria-haspopupmentioned above. Also, we need to update the aria-label so that it begins with the actual displayed text, e.g. "#0066cc. Show how pf-t--global--icon--color--brand--default is derived" (though we should think about shortening this to be more concise, or maybe using a separate icon button to trigger the popover and the value is just plain text)
|
Observation here: Each |
I like the lighter weight tooltip, it is skipped when tab focusing anyhow and the applicable themes are repeated in the actual value popover
Isn't that already the case? At least that's how I've built the Select
I like the pipes because I add additional spacing to line up the theme | mode | contrast |
| position="bottom" | ||
| minWidth="400px" | ||
| > | ||
| <Button |
There was a problem hiding this comment.
Also just need to add aria-haspopup="dialog" to this Button
| const isDisabled = THEME_INVARIANT_CATEGORIES.has(selectedCategory); | ||
|
|
||
| const SelectToggle = (toggleRef) => ( | ||
| <MenuToggle |
There was a problem hiding this comment.
Assuming this is the toggle for filtering by "all" or a single theme, let's also add an aria-label to be similar to that Button I commented on.
Basically, have the aria-label be [selected value], Filter by theme, to mimic what a native Select would convey (the current value + the label)
There was a problem hiding this comment.
Now the toggle will read e.g. "Select theme, current: Default theme | Light | Default contrast" or "Select theme, current: All themes".
There was a problem hiding this comment.
So for this, we should have the currently selected value be at the start of the aria-label to satisfy WCAG 2.5.3, which states "A best practice is to have the text of the label at the start of the name."
There was a problem hiding this comment.
okay thanks, updated
| return ( | ||
| <Select | ||
| aria-label="Select theme" | ||
| role="menu" |
There was a problem hiding this comment.
We shouldn't override the listbox role that Select uses here
| role="menu" |
| <div style={{ marginBlockStart: 'var(--pf-t--global--spacer--xs)' }}> | ||
| {themeNames.map((name) => ( | ||
| <div key={name}> | ||
| <ThemeDisplayLabel themeName={name} /> | ||
| </div> |
There was a problem hiding this comment.
For this Popover can we use a PF plain unordered list? Should keep things visually the same but would make the list of applicable themes easier to parse for AT
There was a problem hiding this comment.
I've decided to keep the current structure since using List and ListItem does add more spacing between the items which I'd have to override in CSS otherwise
There was a problem hiding this comment.
We can revisit this down the line so it's fine if we keep it as-is for now, but the main reason I'd strongly suggest we use a list here is because it's intended to come across as a list of the X themes.
|
@thatblindgeye http://patternfly-tokens.surge.sh/foundations-and-styles/design-tokens/all-design-tokens/ should have latest updates to address your comments, thanks! |
thatblindgeye
left a comment
There was a problem hiding this comment.
@jschuler overall this is looking nice 😎 Had a couple responses to comments above, only 1 I'd say is something we should change now
andrew-ronaldson
left a comment
There was a problem hiding this comment.
Could we do a separate dropdown for Default and Project felt. I found the list hard to scan because of the repeated "Default theme" | "light..." | "...contrast".
Could the Name column include the double hyphen "--pf-t--global..." Good for copying the token names if you're in product. Maybe in a follow up we could add a copy clipboard button for easy access.
The links have "[Semantic] pf-t--global.." at the start. Small nit but could it be "Semantic: --pf-t--global" or "(Semantic) --pf-t--global..."
Do you mean separate out Unified theme options? I am not clear how the interaction should be. How about a horizontal rule instead in the single select? I like your other 2 suggestions can make the change |
There was a problem hiding this comment.
| aria-label="Search for a token" | |
| placeholder="Search for a token" |
a nitpick, but since you can filter and change the table - "all tokens" isn't technically accurate as a placeholder
There was a problem hiding this comment.
or maybe I misunderstood, I can take your wording though
There was a problem hiding this comment.
ah okay ty! Yeah I meant more so because you can filter the table, so the "search for a token" phrase would just be more applicable across any filtered view. looks good 👍
|
feel the same about Andrew's comment about the current menu. I find it a bit complicated, but I also know it's a complex table Or (granted I'm not a designer) @andrew-ronaldson would it be bad to just have 3 menus? And then a "Clear filters" link that would bring back the "All tokens" view? Maybe is too busy or confusing--just musing
|
|
@andrew-ronaldson @edonehoo this is probably a good followup item, wdyt? so we can move a little faster on this PR. I did add some horizontal rules between the sections though which should help a little
|
|
🎉 This PR is included in version 1.16.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
|
late to the game here, but we'll need to update "Unified Theme" in the dropdown to say "Project Felt" @nicolethoen @jschuler @andrew-ronaldson should i open a followup? |
|
@lboehling yes please. So the Select should have entries like |








Design Tokens Table Enhancements
🎯 Key Features
Global Search & Navigation
Theme Management
"Used by" Column
🎨 UI/UX Improvements
📝 Files Changed
Preview
http://patternfly-tokens.surge.sh/foundations-and-styles/design-tokens/all-design-tokens