-
Notifications
You must be signed in to change notification settings - Fork 161
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
chore: Allow drawer content to use full height #2986
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2986 +/- ##
=======================================
Coverage 96.27% 96.27%
=======================================
Files 769 769
Lines 21764 21768 +4
Branches 7038 7039 +1
=======================================
+ Hits 20954 20958 +4
Misses 802 802
Partials 8 8 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approved, but can be simplified further
src/app-layout/drawer/styles.scss
Outdated
@@ -58,6 +58,9 @@ | |||
block-size: 100%; | |||
position: relative; | |||
} | |||
> .drawer-content-wrapper[aria-hidden='false'] { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did not find any condition when aria-hidden=true
is applied on this selector. Does it mean we can remove that part of the selector?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interesting, that feels unexpected because aria-hidden
is set to !isOpen
here. I would've expected it to change when the drawer is opened/closed, like it does for help panel and side nav.
It appears that isOpen
always returns true because it is hardcoded this way. Not sure exactly when or what changed to make the isOpen
condition obsolete. But I have removed this condition from the style.
9c46464
to
6c3925c
Compare
6c3925c
to
90e5f2a
Compare
90e5f2a
to
36b63ba
Compare
36b63ba
to
8757a9f
Compare
8757a9f
to
d6be802
Compare
@@ -20,6 +20,7 @@ | |||
overscroll-behavior-y: contain; | |||
word-wrap: break-word; | |||
pointer-events: auto; | |||
display: flex; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this needed?
The default is flex-direction: row
and I do not think we want this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Discussed in DM, it relies on the default align-items: stretch
behavior to stretch a single child.
Risky practice, but okay
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is only one child, so it's not actually aligning anything horizontally or vertically. But I can add a more explicit definition.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank makes it easier to follow, thanks
Co-authored-by: Boris Serdiuk <[email protected]>
Description
This is a follow up from #2924 that addresses double scrollbar issue in Q chat panel (AWSUI-59823).
It also addresses an issue in Firefox where side nav would have a scrollbar when there was no side nav header present, the original reason for revert. The other issue regarding flashing scrollbar has been extracted from this PR.
Related links, issue #, if available: AWSUI-59823, TT: V1549419424, TT: D169004346,
How has this been tested?
Related screenshot tests added in CR-158701022
Ran through dev pipeline screenshot approvals:
Review checklist
The following items are to be evaluated by the author(s) and the reviewer(s).
Correctness
CONTRIBUTING.md
.CONTRIBUTING.md
.Security
checkSafeUrl
function.Testing
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.