-
Notifications
You must be signed in to change notification settings - Fork 54
Fix leaking props in SubdomainNavBar to remove warning from unit tests #1082
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 leaking props in SubdomainNavBar to remove warning from unit tests #1082
Conversation
🦋 Changeset detectedLatest commit: bf8c2d5 The changes in this PR will be included in the next version bump. This PR includes changesets to release 8 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
🟢 No design token changes found |
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.
Pull Request Overview
This PR prevents unwanted props from appearing in the DOM by removing an unused prop from the component and explicitly destructuring story args.
- Removed the unused
showOnlyOnNarrow
prop fromSubdomainNavBar
. - Updated the Storybook template to destructure and exclude
showSearch
andnumLinks
.
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
File | Description |
---|---|
packages/react/src/SubdomainNavBar/SubdomainNavBar.tsx | Removed the showOnlyOnNarrow prop so it no longer leaks into the rendered DOM. |
packages/react/src/SubdomainNavBar/SubdomainNavBar.stories.tsx | Destructured showSearch and numLinks in the Template signature to avoid prop leakage. |
Comments suppressed due to low confidence (1)
packages/react/src/SubdomainNavBar/SubdomainNavBar.tsx:322
- The
showOnlyOnNarrow
prop was removed from the JSX, but the component's prop interface and any related documentation or tests may still reference it. Consider cleaning up theSubdomainNavBarProps
type and removing any mentions ofshowOnlyOnNarrow
to keep the API contract consistent.
)}
🟢 No visual differences foundOur visual comparison tests did not find any differences in the UI. |
@@ -322,7 +322,6 @@ function Root({ | |||
)} | |||
{hasLinks && !menuHidden && ( | |||
<NavigationVisbilityObserver | |||
showOnlyOnNarrow |
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.
Oh wow, so this wasn't doing anything at all? 😬
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.
No I don't believe so 😅
Summary
Fixes props which were leaking into the DOM, both in Storybook and in the
SubdomainNavBar
component itself.List of notable changes:
SubdomainNavBar.stories.tsx
to prevent props leaking into the DOM of rendered story.showOnlyOnNarrow
prop fromSubdomainNavBar.tsx
to prevent it leaking into the DOM and to fix a related warning in the unit tests.What should reviewers focus on?
showOnlyOnNarrow
prop being used anywhere, and I'm not sure it was ever implemented since being added in SubdomainNavBar overflow fix #451. Another set of eyes on this would be appreciated though.Contributor checklist:
update snapshots
label to the PR)Reviewer checklist:
Screenshots: