fix: Sticky offset in nested AppLayout #3013
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #3013 +/- ##
========================================
Coverage 96.34% 96.34%
========================================
Files 779 779
Lines 21913 21928 +15
Branches 7502 7104 -398
========================================
+ Hits 21112 21127 +15
- Misses 749 794 +45
+ Partials 52 7 -45 ☔ View full report in Codecov by Sentry. |
georgylobko
force-pushed
the
fix/sticky-offset
branch
from
f67ada2 to
23c016d
Compare
georgylobko
requested review from
at-susie,
just-boris and
jkuelz
and removed request for
a team
georgylobko
force-pushed
the
fix/sticky-offset
branch
from
40873b8 to
1b24bf7
Compare
…lVars.stickyVerticalTopOffset var in the dom element
|
|
||
| useLayoutEffect(() => { | ||
| setIsNested(getIsNestedInAppLayout(rootRef.current)); | ||
| }, []); |
There was a problem hiding this comment.
@just-boris you previously suggested to skip this calculation if toolbar is empty, but it turns out this performance optimization might even backfire if we put it as a dependency here, since it will be recalculated every time this prop changes, so I decided to leave it as is
There was a problem hiding this comment.
I do not understand this comment. Could you post here in the comments a possible code how you planned to handle the toolbar and which dependency concerned you?
There was a problem hiding this comment.
useLayoutEffect(() => {
if (toolbarProps) {
setIsNested(getIsNestedInAppLayout(rootRef.current));
}
}, [toolbarProps]);There was a problem hiding this comment.
How about using hasToolbar value. It is boolean and rarely changes:
| if (getComputedStyle(currentElement).getPropertyValue(globalVars.stickyVerticalTopOffset)) { | ||
| return true; | ||
| } | ||
| currentElement = currentElement.parentElement; |
There was a problem hiding this comment.
CSS var values propagate. You do not need to manually walk up
There was a problem hiding this comment.
Technically, the decision suggested by you is better, but it does not work in u tests env so I turned it back
There was a problem hiding this comment.
Okay, so getComputedStyle implementation is JSDOM is more naive.
Let's leave a comment for the future readers this loop is needed for JSDOM.
Fine to keep it because in the real browsers it will exit after the first iteration anyway
|
|
||
| useLayoutEffect(() => { | ||
| setIsNested(getIsNestedInAppLayout(rootRef.current)); | ||
| }, []); |
There was a problem hiding this comment.
I do not understand this comment. Could you post here in the comments a possible code how you planned to handle the toolbar and which dependency concerned you?
|
|
||
| export default function () { | ||
| return ( | ||
| <ScreenshotArea gutters={false}> |
There was a problem hiding this comment.
Need to ensure this dev page passes new integration tests added there: #3063
Co-authored-by: Boris Serdiuk <[email protected]>
Description
Keep table's sticky header within a nested AppLayout on scroll by resetting globalVars.stickyVerticalTopOffset and globalVars.stickyVerticalBottomOffset for nested layout.
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.