fix: Prevent dragging options in Collection preferences from causing page overflow #1072
Conversation
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## main #1072 +/- ##
=======================================
Coverage 92.81% 92.82%
=======================================
Files 606 607 +1
Lines 15914 15913 -1
Branches 5135 5131 -4
=======================================
Hits 14771 14771
+ Misses 1065 1064 -1
Partials 78 78
☔ View full report in Codecov by Sentry. |
| @@ -21,60 +21,54 @@ $border-radius: awsui.$border-radius-item; | |||
| } | |||
| } | |||
|
|
|||
| .sortable-item-toggle { | |||
| .content-display-option-toggle { | |||
There was a problem hiding this comment.
Harmonized internal names with testing utils API: "sortable item" is now "content display option".
| const componentPrefix = 'content-display-option'; | ||
| export const getClassName = (suffix?: string) => styles[[componentPrefix, suffix].filter(Boolean).join('-')]; | ||
|
|
||
| export interface ContentDisplayOptionProps { |
There was a problem hiding this comment.
Following the "presentational component" pattern as documented in the dnd-kit documentation, this component is used in two ways:
- For the options displayed in the list, wrapped in DraggableOption
- While dragging, to render the active (i.e, dragged) item wrapped in the DragOverlay
| box-shadow: awsui.$shadow-container-active; | ||
| border-radius: $border-radius; | ||
| @include focus-visible { | ||
| @include styles.focus-highlight(0px, $border-radius); |
There was a problem hiding this comment.
The second parameter $border-radius was previously missing; this fixes the focus ring border radius.
| flex-grow: 1; | ||
| @include styles.text-wrapping; |
There was a problem hiding this comment.
This fixes the word wrapping in the option label.
| @@ -64,6 +69,43 @@ describe('Collection preferences - Content Display preference', () => { | |||
| await expect(wrapper.findModal()).not.toBeNull(); | |||
| }) | |||
| ); | |||
|
|
|||
| describe('does not cause overflow when reaching the edge of the window', () => { | |||
| const wait = (delay: number) => new Promise(resolve => setTimeout(resolve, delay)); | |||
There was a problem hiding this comment.
Can we use page.pause() instead?
Description
This change fixes 3 things:
1. Page overflow
The unintended behavior depicted in the video below, where dragging an item beyond the right or bottom edge of the browser window would cause overflow:
Screen.Recording.2023-04-18.at.09.38.55.mov
This had been identified previously as a high-priority post-release item, and also reported in #804 (comment) (where the video above is taken from).
This has been fixed by using a drag overlay, which is anyway the recommended way to implement dragging with dnd-kit: https://docs.dndkit.com/api-documentation/draggable/drag-overlay. In this approach, the active item (the one being dragged) is rendered in a portal and a placeholder (blue rectangle) is rendered in its place in the list. The fact that the active item is rendered outside of the modal, and therefore it doesn't have scrollable ancestors anymore, fixes the overflow issue.
2. Border radius of the focus ring
This PR also fixes the border-radius of the focus ring for the active item to use the
border-radius-itemdesign token (change reviewed by design).Before (border radius too small in Visual Refresh, not corresponding to the border radius of the item itself):
After (expected border radius of 8px in Visual Refresh):
3. Word wrapping in the option labels
In extreme cases where the label text is a single word longer than the space available, break the word.
Before:
After:
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.