chore: Render breadcrumbs slot in the skeleton state #2981
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2981 +/- ##
=======================================
Coverage 96.27% 96.27%
=======================================
Files 769 771 +2
Lines 21768 21774 +6
Branches 7039 7039
=======================================
+ Hits 20958 20964 +6
Misses 802 802
Partials 8 8 ☔ View full report in Codecov by Sentry. |
just-boris
force-pushed
the
fix-breadcrumb-skeleton
branch
from
bd73066
to
b13b623
Compare
just-boris
force-pushed
the
fix-breadcrumb-skeleton
branch
6 times, most recently
from
d801ec6
to
cea141b
Compare
just-boris
force-pushed
the
fix-breadcrumb-skeleton
branch
from
cea141b
to
99bf8a2
Compare
just-boris
force-pushed
the
fix-breadcrumb-skeleton
branch
from
99bf8a2
to
c5a8373
Compare
| @@ -281,11 +281,11 @@ describeEachAppLayout({ themes: ['refresh-toolbar'], sizes: ['desktop'] }, () => | |||
| <DestructibleLayout /> | |||
| </> | |||
| ); | |||
| expect(wrapper.find('[data-testid="local-breadcrumbs"]')!.getElement()).toBeEmptyDOMElement(); | |||
| expect(wrapper.find('[data-testid="local-breadcrumbs"]')!.findBreadcrumbGroup()).toBeFalsy(); | |||
There was a problem hiding this comment.
New behavior here – instead of rendering nothing we render breadcrumbs placeholder to be discovered by our funnel metrics collector
| @@ -128,17 +127,16 @@ describe('BreadcrumbGroup Item', () => { | |||
| lastLink.click(); | |||
| expect(onFollowSpy).not.toHaveBeenCalled(); | |||
| }); | |||
|
|
|||
| test('should add a data-analytics attribute for the funnel name to the last item', () => { | |||
There was a problem hiding this comment.
Moved to other tests in the file above
| ghost?: boolean; | ||
| itemIndex: number; | ||
| totalCount: number; | ||
| disableAnalytics?: boolean; | ||
| } | ||
|
|
||
| export const FunnelBreadcrumbItem = React.forwardRef<HTMLSpanElement, FunnelBreadcrumbItemProps>( |
There was a problem hiding this comment.
Refactored FunnelBreadcrumbItem to allow us rendering "no-UI" breadcrumbs we use to collect funnel metrics
| import breadcrumbGroupItemStyles from '../../breadcrumb-group/item/styles.css.js'; | ||
| import breadcrumbGroupStyles from '../../breadcrumb-group/styles.css.js'; | ||
|
|
||
| export const getBreadcrumbLinkSelector = (index: number) => | ||
| `.${breadcrumbGroupStyles['breadcrumb-group']} .${breadcrumbGroupStyles.item}:nth-child(${index}) .${breadcrumbGroupItemStyles.anchor}`; |
There was a problem hiding this comment.
These class names are random and may resolve incorrectly, if breadcrumb on a page runs from a different version than this code.
Replaced with a data-attribute marker, which is more reliable
| export function createWidgetizedComponent<Component extends FunctionComponent<any>>(Implementation: Component) { | ||
| export function createWidgetizedComponent<Component extends FunctionComponent<any>>( | ||
| Implementation: Component, | ||
| Skeleton?: React.ForwardRefExoticComponent<PropsType<Component> & React.RefAttributes<HTMLElement>> |
There was a problem hiding this comment.
New feature here, in addition to the fully working implementation we also receive a skeleton state to render while loading.
just-boris
requested review from
connorlanigan
and removed request for
a team
| */ | ||
|
|
||
| // backward compatibility before this commit: 7a4b7b3e3b1d50830383805a8f4ab6cd93c9701f | ||
| .breadcrumbs-own:not(:empty) + .breadcrumbs-discovered { |
There was a problem hiding this comment.
Not new code, just moved following BreadcrumbsSlot extraction into a separate mini-component
|
It looks like we switched from reading the third breadcrumb item (1-based) to reading the second breadcrumb item (when converted from 0-based indexing): Is this intended? |
|
yes, intentional. The second child is the ellipsis item which is always rendered when it is empty. The existing regression tests confirm that we still resolve to the expected text |
Description
Make breadcrumbs slot rendered even when the app layout widget is in the loading state, because it is important for funnel metrics tracking
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.