-
Notifications
You must be signed in to change notification settings - Fork 153
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
fix: Empty notifications height #2810
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2810 +/- ##
==========================================
- Coverage 96.19% 96.18% -0.01%
==========================================
Files 761 761
Lines 21408 21411 +3
Branches 7310 6927 -383
==========================================
+ Hits 20593 20595 +2
- Misses 762 808 +46
+ Partials 53 8 -45 ☔ View full report in Codecov by Sentry. |
@@ -377,7 +379,7 @@ const AppLayoutVisualRefreshToolbar = React.forwardRef<AppLayoutProps.Ref, AppLa | |||
{!hasToolbar && breadcrumbs ? <ScreenreaderOnly>{breadcrumbs}</ScreenreaderOnly> : null} | |||
<SkeletonLayout | |||
style={{ | |||
[globalVars.stickyVerticalTopOffset]: `${verticalOffsets.header}px`, | |||
[globalVars.stickyVerticalTopOffset]: `${verticalOffsets.header}px + ${resolvedStickyNotifications && notificationsHeight > 0 ? tokens.spaceXs : '0px'}`, |
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.
You may ask why this space is calculated here, not in the computeVerticalLayout
or not just calculate it as part of notifications height.
There are 2 reasons:
- Since now notifications height includes only actual content size, not the wrapper size, we need to take this space somewhere
- It's not feasible to include this to
computeVerticalLayout
, becausetokens.spaceXs
is not anumber
, but css token (string)
If you have a suggestion how to implement it better, let's discuss it in this thread
Temporarily addressed here #2821, closing this one for now as no urgent need |
Description
This issue arises when the notifications in the refresh-toolbar theme are empty. The current implementation relies on the :not(:empty) CSS property, which doesn't work because the notifications DOM node is always present, even when there are no notifications. In this PR, I updated the calculation to match how it's handled in the refresh theme
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
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.