feat: Duplicate - Blue dialog contribution #2946
Conversation
cansuaa
force-pushed
the
cansua/blue-dialog-contribution
branch
from
277560b to
793d280
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2946 +/- ##
=======================================
Coverage 96.45% 96.45%
=======================================
Files 790 790
Lines 22294 22294
Branches 7252 7252
=======================================
Hits 21504 21504
Misses 783 783
Partials 7 7 ☔ View full report in Codecov by Sentry. |
pages/bluedialog/dialog.tsx
Outdated
| const dialogRef = useRef<HTMLDivElement>(null); | ||
| const triggerRef = useRef<HTMLElement | null>(null); | ||
|
|
||
| useEffect(() => { |
There was a problem hiding this comment.
I think it is better to manage the focus transition outside of this component. For instance, you can forward the ref to the dialog's root so that it will be possible to do dialogRef.focus() from the outside code.
There was a problem hiding this comment.
It makes sense, done in the new revision. I didn't move trigger though because it doesn't really have one right now so I will add that when the dialog is getting included into the demo
| .dismiss { | ||
| order: 1; | ||
| padding-inline-end: awsui.$space-container-horizontal; | ||
| } | ||
|
|
||
| .inner-content { | ||
| flex: 1; | ||
| overflow: hidden; // prevent textarea from growing outside of the container | ||
|
|
||
| padding-block-start: awsui.$space-scaled-s; | ||
| padding-block-end: awsui.$space-scaled-l; | ||
| padding-inline-start: awsui.$space-container-horizontal; | ||
| } |
There was a problem hiding this comment.
Moved the padding stylings to dismiss and inner-content because once overflow hidden was passed the focus highlight of left most checkbox was being cut.
| border-end-start-radius: awsui.$border-radius-container; | ||
| border-end-end-radius: awsui.$border-radius-container; | ||
|
|
||
| outline-offset: 5px; |
There was a problem hiding this comment.
Checked with Lin, she also agreed that this one looks better, other way of adding the focus highlight made the dialog look like an alert
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.