Skip to content
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(perf): Prevents settings route from marking "Performance" in sidebar as active #77946

Merged
merged 1 commit into from
Sep 23, 2024

Conversation

gggritso
Copy link
Member

@gggritso gggritso commented Sep 23, 2024

There's a /settings/projects/<project-slug>performance/ URL that activates the "Performance" sidebar item because it's using an inexact URL match. Omit Settings pages the same way other paths do this. Yucky but works for now!

Before:

Screenshot 2024-09-23 at 10 28 15 AM

You can see both "Performance" and "Settings" are active, gross.

There's a `/settings/projects/<project-slug>performance/` URL that
activates the "Performance" sidebar item because it's using an inexact
URL match. Omit Settings pages.
@github-actions github-actions bot added the Scope: Frontend Automatically applied to PRs that change frontend components label Sep 23, 2024
@gggritso gggritso requested review from a team September 23, 2024 14:46
@gggritso gggritso marked this pull request as ready for review September 23, 2024 14:47
@vgrozdanic
Copy link
Member

vgrozdanic commented Sep 23, 2024

Should we do this also for other URLs that do not have !location.pathname.startsWith('/settings/') check?

There might be some other projects which could cause buggy behaviour if the name matches one of our url paths

@gggritso
Copy link
Member Author

@vgrozdanic definitely maybe! I don't know if any other URLs in-app have matching Settings routes (or other conflicting rules overall) I'd rather just patch this one thing for now and use router-native active links if possible and solve this once and for all (though I don't know why we're not doing that and how much work it is to implement that)

Copy link
Member

@ryan953 ryan953 left a comment

Choose a reason for hiding this comment

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

#patterns

@gggritso gggritso merged commit 14db5e8 into master Sep 23, 2024
45 checks passed
@gggritso gggritso deleted the fix/perf/settings-performance-sidebar-active branch September 23, 2024 15:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Scope: Frontend Automatically applied to PRs that change frontend components
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants