Go through each component and check for anything broken, API usage, etc

[benoitgrelard]

We need to make sure all of our components are working as expected

• AccesibleIcon ✅
• Accordion ✅
• Arrow
• AspectRatio ✅
• Avatar ✅
• Checkbox ✅
• Collapsible
• Dialog ✅
• LiveRegion ✅
• Lock ✅
• Popper ✅
• Popover ✅
• Portal ✅
• Radio
• Sheet
• Slider
• Switch 🚧
• Tabs
• ToggleButton ✅
• Tooltip ✅
• VisuallyHidden ✅

[benoitgrelard]

AccessibleIcon – ✅ in #134

• with the span being inline by default, the bounding box isn't correct
  • we should add display: 'inline-block' to the root styles
  • for the bounding box to be correct, the icon (svg) also needs verticalAlign: 'middle'. Not sure how we want to handle this one, considering it's just a child, we could potentially do inline as we're cloning it but then we have potential specificity issues. Perhaps we need a basic Icon primitive to have these styles on or expose a AccessibleIcon.Icon part?

[benoitgrelard]

Accordion – ✅ in #135

• functionally it seems to work fine
• accessibility-wise I'm not entirely sure how it's suppose to announce things
• also, I can see we have an Accordion.Header and Accordion.Trigger but they're not used in the stories, so I'm a bit confused about those…

[benoitgrelard]

Arrow

• only wondering here about the default reset for svg, whether it should be inline-block rather than block 🤔
  • I'll come back to this once I validate their use case with tooltip/popover

[benoitgrelard]

AspectRatio – ✅ in #136

• I know this was already the case previously, but I feel like we might as well have a number as the ratio prop. I don't see much advantage to do ratio='16:9' vs ratio={16/9}. What do we think?
• I feel it's a bit weird to have a part for the inner which is really only an implementation detail…
  • I understand that this is probably just because of the styles that need to be applied to it, because users need to be able to have that part to put styles on there
  • however, I am wondering if we could do it via a descendant selector & > div (or more specific if needed)
  • the main reason I'm wondering this is because I feel like there are already some reset / functional styles that actually can't work straight as inline styles (things like '&:disabled': { pointerEvents: 'none' }, or &:focused { … }) and others.
  • so that's kinda assuming already that people won't be using these with inline styles but rather with static CSS or a CSS-in-JS solution
  • so in that sense, we could then decide to rely on a descendant selector like so, and simplify the whole use case as I don't see much other value in exposing that inner part…

[benoitgrelard]

Avatar – ✅ in #137

• Abbr never renders if we don't provide a src, not sure if that's on purpose
• I'm genuinely confused by the API / usage of that component now that it's moved over to an open component
• Also not sure we need the renderFallback and renderLoading props
• there's already an issue to review that API anyway (Review Avatar API #125)

[benoitgrelard]

Checkbox – ✅ in #106

• use-case wise, it took me a really long time to figure out that I need to put an icon inside the <Checkbox.Icon> for it to work, I just thought it was broken for so long. I suppose that makes sense as this way people can use their own thing but I wonder if we could include something really basic if they don't pass any children to Checkbox.Icon
• API-wise I don't really understand why we need a render prop
• currently I am not able to style the box in focused state (or any state really) as things don't seem to work if I put the Input outside the Box (which I need in order to make them sibling and use the sibling selector. How am I suppose to style the checkbox? Am I missing something?
• I know @jjenzz has changed quite a few things in Rethink Checkbox API #106 (I'm checking all these on main) but actually looking at this now, I'm thinking some of the changes might have been a bit hasty perhaps? ie. same things relating to styling different states of the box itself

[benoitgrelard]

Dialog – ✅ in #171

• The Dialog is always open no matter what value isOpen prop is…
• This one will need more testing once Reset stories for Dialog #111 is done as a lot of changes are happening in there
• I know that's not something we had before either, but I think a missing piece is also the target stuff because there's some accessibility linked to that too probably as to, what was used to open it, etc. something similar to how Tooltip is setup

[benoitgrelard]

LiveRegion – ✅ in #166

• Seems to work pretty well although didn't announce the full sentence in Safari + Voiceover
• Wondering if we should just wrap it internally in VisuallyHidden, what was the thinking here in terms of leaving that up to the user? Is it that in some cases it might be exactly the same as you want in the UI so you'd use that both for the visual and the live region?

[benoitgrelard]

Lock – ✅ in #148

• seems to work fine
• the scroll locking functionality was taken out of that component's responsibility
• need to re-add the old stories for that one

[benoitgrelard]

Popover – ✅ in #158

• can't get the arrow to position properly, seems to be some bugs around that
• renderOnlyWhileOpen why do we need this?
• shouldPortal do we need this? also, might impact positioning as we could make some assumptions before
• positionOverride why do we need this?
• The usage of this component is strange, I have an arrow but no content, etc
• I know that's not something we had before either, but I think a missing piece is also the target stuff because there's some accessibility linked to that too probably as to, what was used to open it, etc.

[benoitgrelard]

Portal – ✅ in #145

• works well
• need to re-add the stories
• need to re-add the tests

[benoitgrelard]

Radio

• all the same points as Checkbox – see Go through each component and check for anything broken, API usage, etc #133 (comment)

[benoitgrelard]

Sheet

• all the same points as Dialog – see Go through each component and check for anything broken, API usage, etc #133 (comment)

[benoitgrelard]

Slider

• on the surface works fine
• found and previously raised a couple of functionality bugs (Slider: clicking on track doesn't move thumb #126, Slider: keyboard only works after having clicked the Thumb #127)
• also noticed some others raised (Slider: review current status and make sure API looks good #25, Slider has been broken since moving into new repo #47)
• code probably needs to be checked for more issues by @jjenzz

[benoitgrelard]

Switch – 🚧 in #178

• a lot of similar points as Checkbox – see Go through each component and check for anything broken, API usage, etc #133 (comment)
• seems to have a lot of code in there that's not necessary anymore, like height checking, automatic translate, etc
• I guess we should discuss what becomes of this type of stuff

[benoitgrelard]

Tabs

• has intermediary Tabs.tsx file that re-exports all from Tabs.primitive.tsx, we should have all code in Tabs.tsx like other components
• seems to work fine functionally
• we don't have the concept of Tabs.Root here which I think is nicer, but we need consistency
• are we relying on aria attributes to style the selected tab?

[benoitgrelard]

ToggleButton – ✅ in #175

• works fine
• are we relying on aria attribute to style toggled state?

[benoitgrelard]

Tooltip – ✅ in #159

• main functionality seems to work fine
• inherits the same issues as Popover – see Go through each component and check for anything broken, API usage, etc #133 (comment)
• it's weird how some components have a xx.Root component and some root reset styles but there's no component rendered here in this case
• the approach with the Tooltip.Target is good. This is what I meant when talking about the Popover or Dialog, we also need something like that for these as there's some attributes to set on the target as well.

[benoitgrelard]

VisuallyHidden - ✅ in #143

• already discussed with @jjenzz, probably needs to be more closed as it's a pure utility thing, no need to modify it
• no prop spreading Kept prop spreading but removed custom props (CS)
• no style Kept styles (CS)
• and we can probably do something cleaner than the bypassInlineStyles by using these styles internally "when" we need them in other components. Removed bypassInlineStyles, styles not applied by default (CS)

[benoitgrelard]

Closing this now as we are tracking on Notion.