- Notifications
You must be signed in to change notification settings - Fork927
feat: show organization name in workspaces table#14547
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
| play: async ({ canvasElement }) => { | ||
| const canvas = within(canvasElement); | ||
| const accessibleTableCell = await canvas.findByRole("cell", { | ||
| // Need whitespace classes because different parts of the element | ||
| // are going in different spans, and Storybook doesn't consolidate | ||
| // these | ||
| name: /org\s?(?:anization)?\s?:\s?Limbus Co\./i, | ||
| }); | ||
| expect(accessibleTableCell).toBeDefined(); | ||
| }, |
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.
Just wanted to make sure that I understand the limitations around decorators: they're basically assumed to always be static values, right? There's no easy way to make a decorator re-render with a new value?
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.
each "decorator" undergoes a small transformation to get turned into a component by storybook. there's no way to force a rerender from the outside, just like any other component. being a component tho, they can call hooks and have state, which is kind of non-obvious.
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.
only thing is that I'm wondering if we really even need the labels. curious what the rationale was.
| org | ||
| <span css={{ ...visuallyHidden }}>anization</span>: |
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.
are screen readers going to handle this split well though? my gut feeling is they say "org anization" with some awful disconnected pronounciation
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.
I tried it with the built-in Mac screen reader, but (1) I think I was just assuming that any elements that are semantically inline elements wouldn't affect pronunciation, and (2) the Mac screen reader isn't even the most popular reader by a long shot
Let me change this just to be on the safe side
| @@ -54,7 +54,7 @@ export const AvatarData: FC<AvatarDataProps> = ({ | |||
| css={{ | |||
| fontSize: 13, | |||
| color: theme.palette.text.secondary, | |||
| lineHeight:"150%", | |||
| lineHeight:1.5, | |||
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.
ah the wonderful subtleties of css. so annoying that this actually means something different. 🙃
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.
CSS is so silly sometimes
@aslilac Sorry I didn't get back to this yesterday, but now that I think about it, I think you're right, and that the labels are unnecessary I guess my worry was that, as we keep cramming multiple values into single table cells, we still need a way to structure the information. Because the HTML spec basically assumes that table cells will basically be like database columns – no more than one value per cell. So anytime we diverge from that, we can't assume that we'll have accessible info just because we're using the correct baseline semantics. I think I'm also just a bit skittish about multiple unlabeled values in general I still think the labels are good for screen readers, but I do think it's overkill for sighted users. Let me switch things around, which should also make it easier to maintain on our end |
c249174 intomainUh oh!
There was an error while loading.Please reload this page.
Closes#14278
Changes made
showOrganizationproperty fromuseDashboardistrueAvatarDatacomponent to make sure that its line height can't inherently incorrectly