Conversation
|
LuLaValva
left a comment
There was a problem hiding this comment.
Among the non-storybook changes I've made, that might need a second look (in order of importance):
- Switched some mandatory
aria-labeltoa11yTextwith TS mandate.- The most iffy one is
<evo-icon-button>since<evo-button>doesn't mandate, but I think it's justified since icon button never has text content
- The most iffy one is
- We had some components with
<@img>attr tags and some with<@image>. I unified them all to<@img>, but maybe<@image>would be better - A good number of attributes that existed in the
Inputtype and were destructured were unused, so I removed them - In some cases
contentwas being rendered as<span><${input}/></span>. To give more control, I switched to<span ...input/>
83e2a78 to
3506827
Compare
3506827 to
51596d1
Compare
| @@ -1 +1 @@ | |||
| <evo-avatar aria-label="Signed out" ...input/> | |||
| <evo-avatar a11yText="Signed out" ...input/> | |||
There was a problem hiding this comment.
Now that we aren't doing pass through for aria-label what happens if:
- aria-label is supplied
- A component requires aria-labelledby? (e.g. carousel using a nearby heading)
p.s. I picked avatar arbitrarily. My comment relates to any component.
There was a problem hiding this comment.
This is a good question! This is a great candidate for ADR, but I just needed to get something out (evo-marko is still in beta until all components have a few more touch-ups)
Right now I'm thinking we should soft-prevent aria-label with TypeScript, but still pass it through. If people want to use aria-labelledby they can pass a11yText="" or maybe a11yText=false alongside it.
I went through every
<evo-*>component's storybook and made sure they were all aligned.While doing this, I uncovered dozens of inconsistencies and bugs in our components and fixed them along the way.
I know this has a bunch of massive sweeping changes, but since
evo-markois still in beta now is the time to do this. I'm excited to see the number of rough edges in our components & stories reduced.We should not merge this yet, because it depends on an experimental and unreleased version of
@storybook/markothat only exists on this PR branch. The goodies I'm hoping to get merged into that package arechangeHandlerto have controlled components hook into storybook controls, with more seamless descriptions"@": {}to scope attr tag attributes more easily and keep them consistent.In the future, we also definitely need a way to control/render
contentin stories and repeatable@attrtags in controls, but I think it's at least much better with what I've implemented so far.