-
Notifications
You must be signed in to change notification settings - Fork 2k
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
Force no site selection on /theme
and /themes
#87463
Force no site selection on /theme
and /themes
#87463
Conversation
This PR does not affect the size of JS and CSS bundles shipped to the user's browser. Generated by performance advisor bot at iscalypsofastyet.com. |
/theme
and /themes
/theme
and /themes
71d8f7e
to
c0ff92f
Compare
/theme
and /themes
/theme
and /themes
@xavier-lc I cannot reproduce this. Am I missing anything? ![]() |
@mmtr I can repro with a clean cache (wiping the indexedDB should be enough). This seems to happen in every environment except dev (of course). 😭 |
@mmtr could you try with different accounts? It happens to me with random testing accounts, but not the main one. |
@xavier-lc I've been able to reproduce it on a incognito window. But I'm unable to reproduce it locally or in Calypso Live as @Copons noted, which makes hard to verify if these changes actually solve the issue. @xavier-lc have you managed to reproduce the issue locally? How have you determined that these changes effectively fix it? |
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.
I think we can try this anyway and see what happens in prod. Code changes make sense, and they don't introduce any regression for me when I test locally
No, I haven't seen it happen locally. I just thought that the change makes sense 😅
👍 |
7bb1758
to
caef01c
Compare
After deploying #87000, I noticed that the left side navigation appeared different on production (showing a selected site) than it did on local/PR preview (showing "All sites"), as you can see on the screenshot:
Proposed Changes
On prod,
CurrentSite
has aselectedSite
object, whereas on local it'snull
, and theselectedSite
prop comes fromstate.ui.selectedSiteId
, so this PR attempts to fix that by setting the selected site to null with thenoSite
middleware.Testing Instructions
/themes?flags=-layout/dotcom-nav-redesign
Pre-merge Checklist