[WNMGDS-2893] Implement Tooltip web component #3249
Conversation
jack-ryan-nava-pbc
added
Status: Work In Progress
| return Tooltip.defaultProps.offset as TooltipProps['offset']; | ||
| }; | ||
|
|
||
| const isPlacementValue = (location: string): location is Placement => { |
There was a problem hiding this comment.
The goal with this function is to enforce some guardrails on the user input for the placement value attribute. This could be overwrought, and I can probably just remove it and replace the type definition in the WrapperProps as string | TooltipProps['placement'].
| title: string | TooltipProps['title']; | ||
| } | ||
|
|
||
| const parseOffset = (offset: string): TooltipProps['offset'] => { |
There was a problem hiding this comment.
Trying to support an array as a string is maybe a bit wrongheaded. Curious if people think this is likely unnecessary. I have seen this prop used rarely in the wild in the React components as well. So maybe this just gets cut out.
There was a problem hiding this comment.
Yeah, I'd argue that this one isn't necessary right now
There was a problem hiding this comment.
Sounds good! To the chopping block it goes.
| it('closes tooltip when trigger focus is lost', async () => { | ||
| jest.useFakeTimers(); | ||
| const { container } = renderTooltip(); | ||
| const tooltip = container.querySelector('.ds-c-tooltip'); |
There was a problem hiding this comment.
I doubt this class name is going to change anytime soon which is why I didn't go through the trouble of assigning a testID for this query.
| renderTooltip(propsWithSlots); | ||
| const tooltipTrigger = screen.getByText(customTooltipText); | ||
| expect(tooltipTrigger).toHaveTextContent(customTooltipText); | ||
| expect(tooltipTrigger).toHaveTextContent(customHeadingText); |
There was a problem hiding this comment.
Since we set both the title and the contentheading slot values, we expect both sections of text to be contained within the tooltip.
| @@ -1,2 +1,4 @@ | |||
| export * from './Tooltip'; | |||
| export * from './TooltipIcon'; | |||
There was a problem hiding this comment.
We export everything mostly to get access to the type definitions. I can tighten this export statement to just reveal the types if folks think that is a better idea?
| }: WrapperProps) => ( | ||
| <Tooltip | ||
| {...otherProps} | ||
| ariaLabel={otherProps.triggerAriaLabel} |
There was a problem hiding this comment.
OK, so I think the way this is setup actually handles all the cases we might be worried about re duplicative aria labelling.
- You can have both
aria-labelandtrigger-aria-labelexist on the component. Thetrigger-aria-labelattribute will still apply to the child trigger element, and thearia-labelvalue will apply only to the parent. - If you apply an
aria-labeland notrigger-aria-labelattribute, the newaria-labelattribute only exists on the parent web component container. It is not applied to the child trigger element.
As the code currently is, I think these are sufficiently distinct. Please let me know if you have any concerns @kim-cmsds @tamara-corbalt @pwolfert
jack-ryan-nava-pbc
requested review from
pwolfert,
kim-cmsds and
tamara-corbalt
jack-ryan-nava-pbc
removed
the
Status: Work In Progress
Summary
How to test
Checklist
[WNMGDS-####] Titleor [NO-TICKET] if this is unticketed work.Type(only one) label for this PR, if it is a breaking change, label should only beType: BreakingImpacts, multiple can be selected.