chore: Expandable component style refactor #3128
Conversation
Explicit ARIA role is unnecessary when roles are implied from Element. If Element is button, then set aria-label string to altText, else aria-label is undefined. Consumers can pass ARIA role and aria-label through props if using Avatar in an informative way.
Replaced <Box> with <Element> to remove 'as' prop and simplify code.
…4-expandable-style-refactor
…4-expandable-style-refactor
Workday/canvas-kit
|
||||||||||||||||||||||||||||||||||||||||
| Project |
Workday/canvas-kit
|
| Branch Review |
william-issue-2954-expandable-style-refactor
|
| Run status |
|
| Run duration | 03m 03s |
| Commit |
|
| Committer | William Stanton |
| View all properties for this run ↗︎ | |
| Test results | |
|---|---|
Failures
|
0
|
Flaky
|
0
|
Pending
|
21
|
Skipped
|
0
|
Passing
|
929
|
| View all changes introduced in this branch ↗︎ | |
UI Coverage
21.32%
|
|
|---|---|
|
Untested elements
|
1522
|
|
Tested elements
|
410
|
Accessibility
99.17%
|
|
|---|---|
|
Failed rules
|
6 critical
5 serious
0 moderate
2 minor
|
|
Failed elements
|
162
|
| marginRight: system.space.x2, | ||
| flexShrink: 0, | ||
| export const expandableAvatarStencil = createStencil({ | ||
| base: { |
There was a problem hiding this comment.
since this renders an avatar, maybe we extend the avatarStencil as well
There was a problem hiding this comment.
I did try this, and then noticed that the <img> didn't work in the Avatar examples. I didn't spend a lot of time digging in and trying to understand why.
There was a problem hiding this comment.
I found a bug, we have to switch the order of the stencil if we move false above true it works as intended: https://github.com/Workday/canvas-kit/blob/master/modules/react/avatar/lib/Avatar.tsx#L221
My guess is true gets called first and never gets to false
| }); | ||
|
|
||
| // When the component is created, it needs to be a button element to match AvatarProps. | ||
| // Once Avatar becomes a `createComponent` we can default the element type to a `div` | ||
| // and the types should be properly extracted | ||
| // Setting altText prop to a default empty string for decorative purposes |
There was a problem hiding this comment.
should we set aria-hidden as well?
There was a problem hiding this comment.
No, I don't believe aria-hidden is necessary. Setting the alt attribute on
elements to an empty string is the best practice, and very effective
for hiding images. In the case of an <svg>, we already use ARIA role
"presentation" which strips the implied 'img' role in an <svg> element.
This gives us the same outcome (hiding the graphic), if we can assume there
isn't any <text> rendered in the <svg>.
There was a problem hiding this comment.
Gotcha, i just asked because we had an instance where a user can add a url, and cypress complains that you can't have an image with empty alt text, so we had to add aria-hidden
There was a problem hiding this comment.
"... can add a url" - what do you mean? Does this turn the image into a linked image... ?
There was a problem hiding this comment.
Yeah if you pass a url to say an image that's hosted, it renders an img element which requires an alt attribute.
|
Looks like the gap / padding seems off Your PR
Master
|
| compound: [ | ||
| { | ||
| modifiers: {position: 'end', isExpanded: 'false'}, | ||
| styles: { | ||
| marginLeft: 'auto', | ||
| transform: 'rotate(180deg)', | ||
| paddingRight: system.space.x3, // do I need these padding changes? | ||
| }, | ||
| }, | ||
| { | ||
| modifiers: {position: 'end', isExpanded: 'true'}, | ||
| styles: { | ||
| marginLeft: 'auto', | ||
| paddingLeft: system.space.x3, // do I need these padding changes? | ||
| }, | ||
| }, | ||
| { | ||
| modifiers: {position: 'start', isExpanded: 'false'}, | ||
| styles: { | ||
| marginRight: system.space.x2, | ||
| transform: 'rotate(90deg)', | ||
| }, | ||
| }, | ||
| { | ||
| modifiers: {position: 'start', isExpanded: 'true'}, | ||
| styles: { | ||
| marginRight: system.space.x2, | ||
| transform: 'rotate(180deg)', | ||
| }, | ||
| }, | ||
| ], | ||
| }); |
There was a problem hiding this comment.
This is for RTL support with using inline for padding and margin.
| compound: [ | |
| { | |
| modifiers: {position: 'end', isExpanded: 'false'}, | |
| styles: { | |
| marginLeft: 'auto', | |
| transform: 'rotate(180deg)', | |
| paddingRight: system.space.x3, // do I need these padding changes? | |
| }, | |
| }, | |
| { | |
| modifiers: {position: 'end', isExpanded: 'true'}, | |
| styles: { | |
| marginLeft: 'auto', | |
| paddingLeft: system.space.x3, // do I need these padding changes? | |
| }, | |
| }, | |
| { | |
| modifiers: {position: 'start', isExpanded: 'false'}, | |
| styles: { | |
| marginRight: system.space.x2, | |
| transform: 'rotate(90deg)', | |
| }, | |
| }, | |
| { | |
| modifiers: {position: 'start', isExpanded: 'true'}, | |
| styles: { | |
| marginRight: system.space.x2, | |
| transform: 'rotate(180deg)', | |
| }, | |
| }, | |
| ], | |
| }); | |
| compound: [ | |
| { | |
| modifiers: {position: 'end', isExpanded: 'false'}, | |
| styles: { | |
| marginInlineStart: 'auto', | |
| transform: 'rotate(180deg)', | |
| paddingInlineEnd: system.space.x3, // do I need these padding changes? | |
| }, | |
| }, | |
| { | |
| modifiers: {position: 'end', isExpanded: 'true'}, | |
| styles: { | |
| marginInlineStart: 'auto', | |
| paddingInlineStart: system.space.x3, // do I need these padding changes? | |
| }, | |
| }, | |
| { | |
| modifiers: {position: 'start', isExpanded: 'false'}, | |
| styles: { | |
| marginInlineEnd: system.space.x2, | |
| transform: 'rotate(90deg)', | |
| }, | |
| }, | |
| { | |
| modifiers: {position: 'start', isExpanded: 'true'}, | |
| styles: { | |
| marginInlineEnd: system.space.x2, | |
| transform: 'rotate(180deg)', | |
| }, | |
| }, | |
| ], | |
| }); |
Summary
Fixes: #2954
Expandable style re-factor is needed with the new cs prop. We will need to update this component to use the new system level tokens via CSS variables.
Release Category
Tokens
Release Note
ExpandableTargetcomponent is now rendering theBaseButtoncomponent, and may have some small impact on the visual styling of the keyboard focus ring.ExpandableIconstencil is extending theSystemIconstencil, and may have had an impact on the chevron icon color.Avatarcomponent where anaria-labelstring was rendered on a generic HTML element that did not have an explicit or implicitroleChecklist
ready for reviewhas been added to PRFor the Reviewer
Where Should the Reviewer Start?
modules/labs-react/expandable/lib/Expandable.tsx
(all subcomponents)
Line 274: modules/react/avatar/lib/Avatar.tsx
Areas for Feedback? (optional)
Testing Manually
Screenshots or GIFs (if applicable)
Thank You Gif (optional)