Conversation
🦋 Changeset detectedLatest commit: eebc08c 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 |
|
Preview deployments for this pull request: storybook - themebuilder - www - |
| * The name of the person the avatar represents. | ||
| */ | ||
| 'aria-label': string; | ||
| 'aria-label'?: string; |
There was a problem hiding this comment.
Is this intended to be optional?
I see we added lots of aria-labels in the stories and other places its used, so contradicting.
If its itended, suggest adding a changeset for this, or else change it back.
There was a problem hiding this comment.
This one is trcky.
We should be able to do:
<Tooltip content='Ola Nordmann'>
<Avatar tabIndex={0}>
<img src='https://placebeard.it/100x100' alt='' />
</Avatar>
</Tooltip>
...in this scenario, the Tooltip will set data-tooltip, which again sets aria-label. But, I'm not sure how to express this in Typescript? :(
There was a problem hiding this comment.
what about adding data-tooltip to the props of Avatar?
There was a problem hiding this comment.
yeah, tried that, but it did not understand that it got tooltip from the parent <Tooltip> element :s
But, we can just have the user set data-tooltip for this component
There was a problem hiding this comment.
yeah that was my thoughs as well 🤔 Might be kind of weird, but at least we know sometimes when to not throw a type error
| }; | ||
|
|
||
| const handleAriaAttributes = debounce(() => { | ||
| for (const el of document.querySelectorAll('[data-tooltip]')) { |
There was a problem hiding this comment.
Maybe as this as a const as we have done in the other helpers?
There was a problem hiding this comment.
unsure what you med by "Maybe as this as a const"
If you refer to the other selectors being getElementsByTagName, these can and should be const as they are "live" HTMLCollection - meaning it automatically contains all added children. querySelectorAll on the other hand is not live - it is a static snapshot, and thus needs to be evaluated on every run to actually contain the relevant elements :)
There was a problem hiding this comment.
I was thinking more in terms of code consistency. Like its done with ATTR_CLICKDELEGATEFOR in clickdelegatefor.ts (or ATTR_TOGGLEGROUP in tooglegroup).
The first const is the "main" data-attr for this function/observer.
There was a problem hiding this comment.
Ah! Right, go ahead <3 :)
Co-authored-by: stianmorsund <stian.rm@gmail.com>
|
We should update |
Yes, supported 💯 |
Which means, in reality we can just deprecate |
6 participants
resolves #1044
resolves #3348
resolves #4369
resolves #4433
Edit by @mimarz: Moved some links to resolving issues from other closed PRs which feat was moved to this PR instead.
Summary
First implementation of a new package that properly uses pure web, and has no dependencies like react.
Related to #1044
You can test this package with the npm tag
test.The preview comment is a bit buried, so putting links here as well:
Checks
pnpm changesetif relevant)