feat: Widgetise app layout skeleton #3396
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3396 +/- ##
========================================
Coverage 96.49% 96.50%
========================================
Files 806 814 +8
Lines 23092 23272 +180
Branches 7987 8022 +35
========================================
+ Hits 22283 22459 +176
+ Misses 802 758 -44
- Partials 7 55 +48 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
…yout-skeleton # Conflicts: # src/app-layout/visual-refresh-toolbar/skeleton/index.tsx
…yout-skeleton # Conflicts: # package.json
| })); | ||
|
|
||
| describeEachAppLayout({ themes: ['refresh-toolbar'] }, () => { | ||
| describeEachAppLayout({ themes: ['refresh-toolbar'], skipInitialTest: true }, () => { |
There was a problem hiding this comment.
We need to skip the mocks applied correctly test in this case because we set widgetMockEnabled = false to test the behavior of skeleton widgets. Since styles now come from widgets and the test relies on those classes, the test will always fail. That’s why we need this extra condition.
| const rootRefInternal = useRef<HTMLDivElement>(null); | ||
| // This workaround ensures the ref is defined before checking if the app layout is nested. | ||
| // On initial render, the ref might be undefined because this component loads asynchronously via the widget API. | ||
| const onMountRootRef = useCallback(node => { |
There was a problem hiding this comment.
This function is newly added. The rest of the code is unchanged and copied from the original index file
georgylobko
requested review from
johannes-weber and
just-boris
and removed request for
a team
| @@ -36,11 +35,11 @@ test('should render refresh-toolbar app layout with the widget flag', () => { | |||
| globalWithFlags[Symbol.for('awsui-visual-refresh-flag')] = () => true; | |||
| globalWithFlags[Symbol.for('awsui-global-flags')] = { appLayoutWidget: true }; | |||
| const content = renderToStaticMarkup(<AppLayout />); | |||
| expect(content).toContain(refreshToolbarStyles.root); | |||
| expect(content).toBe('<div></div>'); | |||
There was a problem hiding this comment.
The condition has changed because the styles now come from widget API, so they don't work with SSR
…voiding layout cumulative shifting)
Description
quip
k8bRAbqrduR3Related 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.