Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #3922 +/- ##
========================================
Coverage 97.17% 97.17%
========================================
Files 854 856 +2
Lines 24988 25242 +254
Branches 8805 8937 +132
========================================
+ Hits 24281 24528 +247
- Misses 659 665 +6
- Partials 48 49 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| }); | ||
|
|
||
| export const registerBottomDrawer = () => { | ||
| awsuiPlugins.appLayout.registerDrawer({ |
There was a problem hiding this comment.
Can we from now on use the new awsuiWidgetsPlugins for all new runtime API?
There was a problem hiding this comment.
Moved to the new runtime api
| if (!root) { | ||
| return; | ||
| } |
There was a problem hiding this comment.
Do we not need to clean up the refs when unmounting?
There was a problem hiding this comment.
No, because refs.close refers to the object link, not its value
| } | ||
| if (props.globalDrawers && !checkAlreadyExists(!!toolbar.globalDrawers, 'globalDrawers')) { | ||
| toolbar.globalDrawersFocusControl = props.globalDrawersFocusControl; | ||
| toolbar.bottomDrawersFocusRef = props.bottomDrawersFocusRef; |
There was a problem hiding this comment.
Why does it go into props.globalDrawers condition instead of its own? What will happen if no global drawers registered?
There was a problem hiding this comment.
Made it go into its own array
| maxGlobalDrawersSizes: Record<string, number>; | ||
| drawers: ReadonlyArray<InternalDrawer>; | ||
| drawersFocusControl: FocusControlState; | ||
| bottomDrawersFocusControl: FocusControlState; |
There was a problem hiding this comment.
AppLayoutInternals is the legacy widget interface which we will remove once all consumers update.
All new features should live AppLayoutWidgetizedState
| "activeAiDrawerSize": 0, | ||
| "activeDrawer": undefined, | ||
| "activeDrawerSize": 290, | ||
| "activeGlobalBottomDrawerId": null, |
There was a problem hiding this comment.
Why is there a diff in the snapshot? Will it cause any impact in the widget?
7ef78e0 to
d3771c6
Compare
7d952c5 to
2b86095
Compare
2b86095 to
27968fe
Compare
ad4ad2a to
7055078
Compare
87d7745 to
91da9d8
Compare
be2861b to
1ca8827
Compare
just-boris
left a comment
There was a problem hiding this comment.
The comment about snapshot diff is blocking, everything else is FYI
| useEffect(() => { | ||
| if (activeGlobalBottomDrawerId) { | ||
| openBottomDrawersHistory.current.add(activeGlobalBottomDrawerId); | ||
| } | ||
| }, [activeGlobalBottomDrawerId]); |
There was a problem hiding this comment.
Ok, I will resolve this thread for now, but please make a follow up
| getMaxHeight: useStableCallback(() => { | ||
| getMaxHeight: useCallback(() => { |
There was a problem hiding this comment.
This smells like a beginning of the problem, but up to you if you are comfortable with that risk or not
src/internal/plugins/widget/index.ts
Outdated
|
|
||
| /** | ||
| * Registers a new runtime drawer to app layout | ||
| * Registers a new ai runtime drawer to app layout |
There was a problem hiding this comment.
Let's call it "left" in the comment too.
Not for this PR, but in general, we should look for removing "ai" from the naming, because it is not relevant. "Global" drawers may contain "AI" stuff too
| }, | ||
| "AppLayoutSplitPanelDrawerBottomImplementation" => { | ||
| "appLayoutInternals": { | ||
| "widgetizedState": { |
There was a problem hiding this comment.
⛳️ There is still a diff in the snapshot even after the change is applied. It seems like more cross-boundary changes need to be fixed
…ize changes in the legacy contract
Description
This PR introduces a new panel within existing
AppLayoutcomponent. It does not affect any publicly available api. The only way to register it is using the existing plugin apiregisterDrawerwith an additional parameterposition: "bottom".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.