Skip to content

Fix: Fixed styling tab not opening on themes without style variations on mobile & desktop #67537

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.

Sign up for GitHub

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

Merged
ramonjd merged 1 commit into from
Dec 3, 2024
Merged

Fix: Fixed styling tab not opening on themes without style variations on mobile & desktop #67537

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 7 additions & 1 deletion packages/edit-site/src/components/sidebar-navigation-screen-global-styles/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,13 @@ export function SidebarNavigationItemGlobalStyles( props ) {
/>
);
}
return <SidebarNavigationItem { ...props } />;
return (
Copy link
Member

@ramonjd ramonjd Dec 3, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for getting this PR up! I'll test further later today.

I'm wondering if we need the hasGlobalStyleVariations condition at all. 🤔

E.g.,

export function SidebarNavigationItemGlobalStyles( props ) {
	const { name } = useLocation();
	return (
		<SidebarNavigationItem
			{ ...props }
			to="/styles"
			uid="global-styles-navigation-item"
			aria-current={ name === 'styles' }
		/>
	);
}

I'll investigate later.

Also, not related to this PR, but there's a persistent JS error for sidebar items on mobile: #67467 (comment)

Just mentioning in case you see it and are wondering 👍🏻

Copy link
Contributor Author

@Mayank-Tripathi32 Mayank-Tripathi32 Dec 3, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought about removing the condition too. But after looking at the original code, I saw that the condition was there and the uid was missing in the default return. To keep the code consistent and working as expected, I decided to leave it as is.

Copy link
Member

@ramonjd ramonjd Dec 3, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, thanks for the context. That's a good point. Anyway, I'll test as soon as I can today and we'll aim to get the fix in ASAP.

Copy link
Member

@ramonjd ramonjd Dec 3, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I still think a lot of this code is superfluous and doubles up on things.

For example, most of the props could be passed to SidebarNavigationItemGlobalStyles in MainSidebarNavigationContent.

			<SidebarNavigationItemGlobalStyles
				to="/styles"
				uid="global-styles-navigation-item"
				icon={ styles }
			>
				{ __( 'Styles' ) }
			</SidebarNavigationItemGlobalStyles>

With the same effect.

Removing the condition doesn't have any side effects as far as I can see, but I agree that we can take the cautious route to fix the bug.

I can get a follow up to clean up the code and we can get folks who have commented on recent changes to routing here to chime in.

cc @youknowriad and @t-hamano for visibility

<SidebarNavigationItem
{ ...props }
to="/styles"
aria-current={ name === 'styles' }
/>
);
}

export default function SidebarNavigationScreenGlobalStyles() {
Expand Down
Loading