-
Notifications
You must be signed in to change notification settings - Fork 156
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
fix: Prevent breadcrumbs from flickering when rendered inside app layout toolbar breadcrumbs slot #2895
Conversation
export const BreadcrumbsSlotContext = | ||
awsuiPluginsInternal.sharedReactContexts.createContext<BreadcrumbsSlotContextType>(React, 'BreadcrumbsSlotContext'); |
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 is needed, because by default React.createContext
produces a new context for each call. In our case, there could be multiple copies of this file, which will not allow context providers connect to its consumers.
We solve it by caching context instances by their name
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.
But we are guaranteed to have only one copy of SharedReactContexts —do I understand correctly?
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.
Yes, because awsuiPluginsInternal
object is deduplicated.
components/src/internal/plugins/api.ts
Lines 62 to 63 in 01aecc9
const existingApi = findUpApi(win); | |
win[storageKey] = installApi(existingApi ?? {}); |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2895 +/- ##
========================================
Coverage 96.21% 96.21%
========================================
Files 761 763 +2
Lines 21511 21531 +20
Branches 7366 6972 -394
========================================
+ Hits 20697 20717 +20
- Misses 761 806 +45
+ Partials 53 8 -45 ☔ View full report in Codecov by Sentry. |
@@ -108,15 +115,17 @@ describeEachAppLayout({ themes: ['refresh-toolbar'], sizes: ['desktop'] }, () => | |||
); | |||
}); | |||
|
|||
test('when breadcrumbs are rendered in multiple slots, the last one takes precedence', async () => { | |||
await renderAsync( | |||
test('when breadcrumbs are rendered in multiple slots, the one in the breadcrumbs slot takes precedence', 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.
Technically, this is a breaking change. But this the new expected one, we should be fine.
c0825dc
to
7d6ea78
Compare
…out toolbar breadcrumbs slot
7d6ea78
to
cfc86a8
Compare
…out toolbar breadcrumbs slot (#2895)
Description
Follow up for #2892.
When breadcrumbs are already rendered in the expected slot, there is no need for the runtime API interactions. This allows for a fast track without additional re-renders.
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
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.