-
Notifications
You must be signed in to change notification settings - Fork 184
chore: Refactor widgetized app layout implementation #3721
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
base: main
Are you sure you want to change the base?
Conversation
4f3ab6b
to
30608ee
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3721 +/- ##
========================================
Coverage 97.01% 97.01%
========================================
Files 825 834 +9
Lines 23963 24140 +177
Branches 8006 8484 +478
========================================
+ Hits 23248 23420 +172
+ Misses 709 671 -38
- Partials 6 49 +43 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
d1d1054
to
03eb8db
Compare
064892b
to
d0ea796
Compare
6a6fc35
to
c68a088
Compare
dded29c
to
d73f905
Compare
d73f905
to
aaff79b
Compare
54326cb
to
2ef1b2c
Compare
2ef1b2c
to
5da31c1
Compare
8e8a906
to
e50070e
Compare
e50070e
to
4b30399
Compare
4b30399
to
eddef65
Compare
0910fc6
to
45c0e9b
Compare
// used for local dev / testing | ||
interface CustomFlags { | ||
appLayoutDelayedWidget?: boolean; | ||
} |
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.
Example of this flag in action: https://d21d5uik3ws71m.cloudfront.net/components/0910fc65859f795dbdd5b95eeebd2b6809daa93a/dev-pages/index.html#/light/app-layout/default?appLayoutWidget=true&appLayoutDelayedWidget=true
Notice how the side panels loaded with a delay
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.
Is CustomFlags should only be used to test, Shall we rename it like awsui-testing-flags
?
@@ -35,7 +36,7 @@ describe('AppLayoutToolbar component', () => { | |||
); | |||
}); | |||
|
|||
test('triggerless navigation', () => { | |||
test('triggerless navigation', async () => { |
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.
Here and in some cases below some tests have become async
This is needed because focus behavior is async now. I checked with a11y squad, the change is backward compatible
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.
We have snapshot tests on internal API between app layout parts.
This file represents the API between the old parts (expected to be deprecated and removed some day).
The other files represent the new API, the future proof one
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.
This file is the HTML rendered before the async part is loaded. All the red text here means this code moved to the async part of the codebase – this is the main point of this change
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.
This file was in the mainline already, not really new code: https://github.com/cloudscape-design/components/blob/main/src/app-layout/visual-refresh-toolbar/multi-layout.ts
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.
Extracted code from this file into a hook: https://github.com/cloudscape-design/components/blob/main/src/app-layout/visual-refresh-toolbar/index.tsx
This is needed to make it asynchronously loaded as well
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.
Minus 500 lines in this file, moved to other places you have seen above
stateManager.current.setState = (appLayoutState, skeletonAttributes, deduplicationProps, mergeProps) => { | ||
setAppLayoutState(appLayoutState); | ||
setSkeletonAttributes(skeletonAttributes); | ||
setDeduplicationProps(deduplicationProps); | ||
setDeduplicator({ fn: mergeProps }); | ||
}; |
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.
Important place no. 1 – this is how we synchronise the local and async parts of this code. See stateManager.current.setState
above to see where this is called
useEffect(() => { | ||
stateManager.current.hasToolbar = hasToolbar; | ||
stateManager.current.setToolbar?.(hasToolbar); | ||
}, [stateManager, hasToolbar]); |
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.
Important place no. 2 – here we send information back to the async part. Find the other instance of stateManager.current.setToolbar
so see where it is used
chore: Move skeleton elements attributes to a widgetized hook chore: Move skeleton slots to widgets chore: Widgetize all remaining components feat: AppLayout state as widget chore: small fix chore: small fix chore: Small refactoring chore: Pull react-reverse-portal as src util chore: Revert unrelated changes in runtime-drawers.page.tsx fix: SSR tests fix: Skeleton part tests feat: Deliver skeleton slots attributes as a widget part fix: Unit tests chore: Update snapshots chore: Tests for reverse portal chore: Additional tests for reverse portal fix: Integ tests chore: Skip hash for class names in widget contract tests fix: Tests chore: Wrap up chore: Fallbacks for app layout component when is in loading state (avoiding layout cumulative shifting) chore: Delay breadcrumbs rendering until app layout state is loaded feat: Event-base prop passing Fix u tests and remove unused reverse-portal feat: Split app layout state into widgetized and built-in parts chore: Merge expanded mode chore: Merge from main fix: useMergeRefs import fix: remove unnecessary import fix: Navigation fallback for useMultiAppLayout chore: Refactoring, fixed tools test chore: Refactoring, fixed skeleton test chore: Update snapshots fix: header-variant test fix: toolbar test fix: multi-layout.test.tsx fix: slots.test.tsx fix: desktop.test.tsx chore: Update snapshots fix: split-panel.test.tsx fix: main.test.tsx fix: drawers.test.tsx fix: common.test.tsx fix: analytics-metadata.test.tsx chore: Update snapshots chore: Update snapshots feat: Async focus control chore: Migrate useUniqueId import to @cloudscape-design/component-toolkit fix: onMount type issue feat: Introduce useAsyncFocusControl chore: Clean up style hash in widget-contract-split-panel.test.tsx chore: Cleanup multi-layout-with-hidden-instances-iframe.page.tsx chore: Small test adjustment chore: Suppress an overlay for runtime errors in webpack config fix: Eslint error fix: AppLayoutPartLoader mounting issue chore: Increased a timeout for multi-layout.test.tsx chore: Types for a state manager chore: Wrapping up (type adjustments, snapshot upd) chore: Turn focusSplitPanel into async function (wait for an element before focusing on it) feat: Custom feature flag appLayoutDelayedWidget to test async widget behavior chore: Integ test for checking ensure the layout is not shifting after widget loading chore: Upd snapshot tests fix: Duplicate 'client' key in webpack.config.base.cjs fix: analytics metadata
45c0e9b
to
dd43ba4
Compare
dd43ba4
to
16e11a9
Compare
Great job Boris and Georgii! I can't find any obvious issues so I'm good with merging this. Will wait an give Yueying the time to review this as well though. |
await testFn(page); | ||
}); | ||
|
||
describe.each(['refresh-toolbar'] as const)('%s', theme => { |
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.
Remove theme
too?
splitPanelPosition?: string; | ||
screenSize?: (typeof viewports)['desktop' | 'mobile']; | ||
disableContentPaddings?: string; | ||
theme: string; |
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.
It seems we do not needed splitPanelPosition
disableContentPaddings
and theme
here?
// used for local dev / testing | ||
interface CustomFlags { | ||
appLayoutDelayedWidget?: boolean; | ||
} |
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.
Is CustomFlags should only be used to test, Shall we rename it like awsui-testing-flags
?
|
||
export const TopContentSlotImplementation = ({ appLayoutProps, appLayoutState }: SkeletonPartProps) => { | ||
if (!isWidgetReady(appLayoutState)) { | ||
return null; |
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.
This part loading can also make page shift, why does it not have a skeleton like beforeMain?
Description
Revisit what is dynamically loaded and what is always a build-time dependency
Related links, issue #, if available: n/a
How has this been tested?
Yes
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.