fix: Render Visual Refresh background height in sync with content #1331
Conversation
This reverts commit efd419e.
This reverts commit 240f793.
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## main #1331 +/- ##
==========================================
+ Coverage 93.47% 93.48% +0.01%
==========================================
Files 624 625 +1
Lines 16778 16810 +32
Branches 5550 5561 +11
==========================================
+ Hits 15683 15715 +32
Misses 1022 1022
Partials 73 73
☔ View full report in Codecov by Sentry. |
…ize (#1311) Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: Rúben Carvalho <[email protected]>
Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: Abdallah AlHalees <[email protected]> Co-authored-by: Timo <[email protected]>
…lter to top level (#1313)
…n charts to top level (#1319)
…tor to top level (#1320)
| expect(isDynamicOverlapDisabled()).toBe('false'); | ||
| }); | ||
|
|
||
| test('sets dynamic overlap when height is higher than 0', () => { |
There was a problem hiding this comment.
Could we have a test case where the height dynamically changes (for example, from 800 to 0) and we check for the expected values of isDynamicOverlapSet and isDynamicOverlapDisabled?
Or is this not something that can happen?
There was a problem hiding this comment.
It's not impossible that these state variables change during the lifecycle of an application but I think it should be rather rare. Some of these events should happen:
- The height of a child component that uses the dynamic overlap hook changes from 0 to non-0, or the other way around from non-0 to 0
- A child component that uses the dynamic overlap hook is added or removed
contentHeaderis passed and then not passed anymore, or the other way around- The value of the
disableDynamicOverlapproperty passed to the app layout changes
I just added a test that checks these state variables after re-rendering, based on the case 2 above. Case 1 would be tricky to test because we would need to simulate a call to the resize observer hook.
There was a problem hiding this comment.
Alright, that makes sense!
| element?.style.setProperty(customCssProps.overlapHeight, `${height}px`); | ||
| } | ||
| }, | ||
| [layoutElement, props.contentHeader, props.disableContentHeaderOverlap] |
There was a problem hiding this comment.
props.contentHeader is a ReactNode, which is rarely stable between renders anyway. This means the function will update identity each render, making useCallback useless.
If it's just provided to check for the existence of the property rather than the contents, maybe you could define a variable outside the function and use it internally? That would make it safer.
const hasContentHeader = !!props.contentHeader;
useCallback(() => { ... }, [layoutElement, hasContentHeader, props.disableContentHeaderOverlap]);
There was a problem hiding this comment.
Updated as per your suggestion.
| * Using React state for `isDynamicOverlapSet` and `isDynamicOverlapDisabled` is less problematic | ||
| * because they will rarely change within the lifecycle of an application. | ||
| */ | ||
| const element = typeof layoutElement === 'function' ? layoutElement(null) : layoutElement?.current; |
There was a problem hiding this comment.
Not sure what layoutElement(null) does here. It just unsets the ref? I think it works here just because of internal implementation details, since layoutElement happens to be an ObjectRef usually. If we're using the ref for things other than resize observer, maybe another ref property (typed as ObjectRef) is needed.
There was a problem hiding this comment.
This variable is returned by useContainerQuery which returns React.Ref<any>, which in turn is RefCallback<any> | RefObject<any> | null, but its value is set only by the Layout component here so we know it's going to be an ObjectRef and not a RefCallback.
If it looks better, we could as well just do nothing if it is a function.
There was a problem hiding this comment.
Not ideal, but it does have a comment and it works for now. Not going to treat it as blocking.
There was a problem hiding this comment.
Should layoutElement somehow end up being used as a RefCallback, it will also cause this useCallback to recreate the updateDynamicOverlapHeight function on every render.
Anyway, it seems unlikely, so it is also non-blocking for me.
|
|
||
| useLayoutEffect( | ||
| function handleDynamicOverlapHeight() { | ||
| const getElement = useCallback(() => overlapElementRef.current, [overlapElementRef]); |
There was a problem hiding this comment.
😭 I'm so not a fan of this ResizeObserver hook API
Description
Alternative approach to #1344, a bit less orthodox (adding imperative DOM updates) but more performant.
As a long-term fix, we want to refactor app layout so that resize observer calls are not necessary to keep the content and the background in sync.
Ticket: AWSUI-21410
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.