MNTOR-5033 - Remove SidebarNavigationRedesign flag #6193
Conversation
...pp/(proper_react)/(redesign)/(authenticated)/user/(dashboard)/settings/SettingsPage.test.tsx
Outdated
Show resolved
Hide resolved
...er_react)/(redesign)/(authenticated)/user/(dashboard)/settings/SettingsPageRedesign.test.tsx
Outdated
Show resolved
Hide resolved
| return ( | ||
| <div | ||
| /* c8 ignore next */ | ||
| className={`${styles.container} ${props.variant ? styles[props.variant] : ""}`} |
There was a problem hiding this comment.
Never had to write a unit test here before, not sure why it's acting up now.
There was a problem hiding this comment.
That's because the component was removed from SettingsContent, which appears to be the only place the variant prop was used. I think that's a sign to remove the whole variant code (i.e. the .secondary class and the references in here), instead of adding the c8 ignore.
There was a problem hiding this comment.
You're right - that variant wasn't being used anywhere. Remove the prop and condition in 6bfc7a3, thank you!
Vinnl
left a comment
Vinnl
left a comment
There was a problem hiding this comment.
Well underway, but there are a couple more cleanup chores that need to happen, I think.
...t)/(redesign)/(authenticated)/user/(dashboard)/settings/stories/SettingsRedesign.stories.tsx
Show resolved
Hide resolved
...er_react)/(redesign)/(authenticated)/user/(dashboard)/settings/SettingsPageRedesign.test.tsx
Outdated
Show resolved
Hide resolved
...pp/(proper_react)/(redesign)/(authenticated)/user/(dashboard)/settings/SettingsPage.test.tsx
Outdated
Show resolved
Hide resolved
src/app/(proper_react)/(redesign)/(authenticated)/user/(dashboard)/dashboard/Dashboard.test.tsx
Show resolved
Hide resolved
| case "manage-account": | ||
| return <SettingsPanelManageAccount {...props} />; | ||
| case "edit-profile": | ||
| if (props.enabledFeatureFlags.includes("EditScanProfileDetails")) { |
There was a problem hiding this comment.
This appears to be removing a different feature flag?
There was a problem hiding this comment.
Oops you're right, that was me trying to test something. Reverting in 07200db
...er_react)/(redesign)/(authenticated)/user/(dashboard)/settings/SettingsPageRedesign.test.tsx
Outdated
Show resolved
Hide resolved
| it("changes the active tab", async () => { | ||
| const user = userEvent.setup(); |
There was a problem hiding this comment.
Is this old UI that no longer exists?
There was a problem hiding this comment.
This test was for the deprecated sidebar navigation, which is being removed in this PR. Its removal doesn’t affect Jest coverage, so it’s no longer needed.
| /* c8 ignore next */ | ||
| className={`${styles.toolbar} ${props.enabledFeatureFlags.includes("SidebarNavigationRedesign") ? styles.hasBackground : ""}`} | ||
| > | ||
| <nav className={`${styles.toolbar} ${styles.hasBackground}`}> |
There was a problem hiding this comment.
Can we now remove the .hasBackground class and just roll those styles into .toolbar?
|
@Vinnl Sorry it took me a while to get back to this. All of your comments should have been addressed now - let me know if you need me to walk you through it. |
* MNTOR-4178 - Remove EditScanProfileDetails feature flag * update basetest * fix build errors * pass empty feature flag array to settings wrapper * Remove EditScanProfileDetails flag logic and restore FreeOnly branching for Edit Info nav link * add tests
|
@codemist Sorry, now I didn't get around to it due to the shutdown timecrunch. I see you just marked this as draft, I assume that's because of the in-progress merge? Do you need help with that? |
References:
Jira: MNTOR-5033
Figma:
Description
Screenshot (if applicable)
Not applicable.
How to test
Checklist (Definition of Done)