feat: wire motion trigger into components - #4941
Conversation
mxschll
changed the title
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #4941 +/- ##
=======================================
Coverage 97.66% 97.66%
=======================================
Files 959 959
Lines 31345 31364 +19
Branches 11579 11591 +12
=======================================
+ Hits 30614 30633 +19
+ Misses 724 685 -39
- Partials 7 46 +39 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
mxschll
changed the title
01b436b to
0d0aeef
Compare
mxschll
changed the title
GitHub Actions
Inactive
| ref={ref} | ||
| startIcon={startIcon} | ||
| endIcon={<InternalIcon name="angle-right" />} | ||
| endIcon={<InternalIcon name="angle-right" nativeAttributes={{ 'data-awsui-motion-target': '' }} />} |
There was a problem hiding this comment.
We only want to play animation for the "angle-right" and not custom provided icons, as hovering + focus is coupled with the angle icon action.
| 'columnDefinitions.editConfig.editIconAriaLabel', | ||
| column.editConfig?.editIconAriaLabel | ||
| )} | ||
| nativeAttributes={{ 'data-awsui-motion-target': '' }} |
There was a problem hiding this comment.
The table header is clickable when it is sortable. Clicking on the header causes its sorting direction to change. I believe we should not make the edit icon animate - as it creates confusion.
There was a problem hiding this comment.
I think it's valid that the icon has the animation here as its an interactive element, similar like a button. But I agree that when I tried it out, it felt a bit odd. Removing it as it can be added at a later stage again if necessary.
| > | ||
| <span className={styles['body-cell-editor-icon']}> | ||
| <Icon name="edit" /> | ||
| <Icon name="edit" nativeAttributes={{ 'data-awsui-motion-target': '' }} /> |
There was a problem hiding this comment.
When a cell is editable - then the entire cell is hover-able and clickable. The semantic button in this case should not act as trigger. Instead - the cell should. Alternatively - we can ignore this completely, as edit button is only visible on hover anyways.
| onPointerDown={onPointerDown} | ||
| onClick={onClick} | ||
| onKeyDown={onKeyDown} | ||
| {...(variant === 'drag-indicator' |
There was a problem hiding this comment.
Why do we only support one variant here?
There was a problem hiding this comment.
The other ones are just horizontal or vertical lines (except the ones with 2 lines) and they would fall back to the default shrinking animation which is almost perceptible for these icons which didn't make sense to animate. The drag-indicator is also the only one with it's custom animation.
| <InternalIcon | ||
| name={isOneTheme ? 'angle-down' : 'caret-down-filled'} | ||
| size={isOneTheme ? 'x-small' : 'normal'} | ||
| nativeAttributes={{ 'data-awsui-motion-target': '' }} |
There was a problem hiding this comment.
Should we also animate consumer-provided icons and external link icon in dropdown items?
| // tabindex=-1 so we can focus them when necessary. | ||
| tabIndex={filteringEnabled ? -1 : highlighted ? 0 : -1} | ||
| ref={triggerRef} | ||
| data-awsui-motion-trigger="hover" |
There was a problem hiding this comment.
I noticed that expand toggles in button dropdown category items also animate when the item is highlighted with the keyboard. How does this work?
There was a problem hiding this comment.
Semantically, focus can never cover more area than hover. Hovering any part of the region hovers the region, while focus lands on exactly one element, which is either the region itself or a descendant of it. Thats why hover is implicitly hover + focus-visible. But we can move the focus trigger to the children if necessary like in the action card.
We could write it explicit everywhere: `data-awsui-motion-trigger="hover focus" but I decided not to because "hover focus" implies "hover" alone is a valid state, but it never is.
| title: __title ?? ariaLabel, | ||
| className: buttonClass, | ||
| onClick: handleClick, | ||
| 'data-awsui-motion-trigger': 'hover', |
There was a problem hiding this comment.
What these are for: is it for internal buttons only? Why only hover trigger?
There was a problem hiding this comment.
As per comment above: "hover" implicitly is "hover + focus" because it enforces focus parity. The focus token only exists for designating the focus owner when it's nested inside the hover region.
2 participants
Adds motion trigger and target attributes to components.
Context:
b9VcPdMJ5zRn/NVoiJ1X1df9a.Description
Related links, issue #, if available: n/a
How has this been tested?
Review checklist
The following items are to be evaluated by the author(s) and the reviewer(s).
Correctness
CONTRIBUTING.md.CONTRIBUTING.md.Security
checkSafeUrlfunction.Testing
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.