fix(Tooltip): only set aria-label if there is no accessible text, and deprecate type prop#4898
fix(Tooltip): only set aria-label if there is no accessible text, and deprecate type prop#4898unekinn wants to merge 7 commits into
type prop#4898Conversation
🦋 Changeset detectedLatest commit: 7fcb60a The changes in this PR will be included in the next version bump. This PR includes changesets to release 5 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Preview deployments for this pull request: storybook - |
|
Oh, nice work! ⭐ Though, I must say I agree a bit with you - my immediate feeling is that we should not rely on Reacts inner workings, ship extra JS or extend "framework components" (I hope we'll be able to add more than React soon), so I'd prefer client-side-logic for this one |
|
Maybe we could add a play function to open the tooltips for chromatic visual tests?
Lets maybe try without first? It does look complicated and cumbersome to maintain. We can revaluate if it becomes a problem for users downstream. We want the React components to be as simple as possible to ensure wide compability and behaviour across all web framework/libraries. |
Good idea, I will do that.
Agreed, I only added this because of the following comment in the original code: However, we have a lot of other components where the |
tested were not set
77c115a to
da9a08d
Compare
|
@mimarz I have removed the SSR-specific code and added some examples – I finally managed to retain the focus border in the existing snapshots too 😅 |
Summary
typeprop, as it no longer does anything (since v1.12.0 rewrite to usedesignsystemet-web)aria-descriptionin that case. If there is no accessible text,aria-labelwill be used as before.NOTE: The complicatedhasAccessibleText()function is only necessary if we want to keeparia-label/aria-descriptionserver rendered (except in case of React Suspense, in which case it will be deferred to the custom element). Writing it was a nice learning experience, but it is a lot more complicated than the same logic which exists in the custom element client side, and I'm not sure it's worth the hassle?EDIT: This is removed, and now only done on the client
Checks
pnpm changesetif relevant)