-
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
Hook Sites Dashboard into All Sites selector #65462
Conversation
Here is how your PR affects size of JS and CSS bundles shipped to the user's browser: App Entrypoints (~4371 bytes added 📈 [gzipped])
Common code that is always downloaded and parsed every time the app is loaded, no matter which route is used. Sections (~121004 bytes removed 📉 [gzipped])
Sections contain code specific for a given set of routes. Is downloaded and parsed only when a particular route is navigated to. Async-loaded Components (~5325 bytes added 📈 [gzipped])
React components that are loaded lazily, when a certain part of UI is displayed for the first time. Legend What is parsed and gzip size?Parsed Size: Uncompressed size of the JS and CSS files. This much code needs to be parsed and stored in memory. Generated by performance advisor bot at iscalypsofastyet.com. |
59f6172
to
0b160fd
Compare
@wojtekn Code-wise, this seems fine to me. When I go to the Site Selector and pick 'All My Sites', though, I see the Sites Dashboard at Is this intended? I expected the background color to be white. |
Thanks for catching that. It's not intended. I've just adjusted the class name based on the section group name. |
@wojtekn It looks like your change broke the styling on |
@danielbachhuber, I think we won't need that route if we hook Sites Dashboard into |
Why not redirect to |
IMO this PR seems cleaner than this approach #65524. And since the Dashboard is similar to SitesComponent, it seems fitting they should be mounted the same place using feature flag to determine which to mount. If there are no other comments on this PR its good to go for me 👍 |
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.
Good to go for me but let's hear from others.
I've updated this PR:
|
dd1e358
to
713645d
Compare
Component is now rendered under another section, so we adjust the class name that is based on the section group name.
fae8c50
to
1b36489
Compare
Oh no! I accidentally pushed to your branch while I was trying something out. It should be back the way it was before (except I resolved a merge conflict due to |
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.
Left a few notes below. Sorry for flip flopping so much, but I'm a bit worried about increasing the bundle size so much. I had thought that it would have only increased the bundle for users who were navigating directly to /sites
or /sites/:id
, but in reality it's increasing the bundle for many many sections. So perhaps the #65524 option is better after all 😬.
I've implemented another idea in #65625 for your consideration:
- It is like Hook Sites Dashboard into /sites routing #65524 in that it changes the "All My Sites" link
- It leaves the
/sites
and/sites-dashboard
routes as they are. I know that was one of the definitions of done in the Sites Management Dashboard: navigate to dashboard from the "All My Sites" button in the Calypso sidebar #65181 task, but I think trying to achieve that is making these PRs more tricky than they site to be - It allows the dashboard to return users back to the place they came from (like
/media
or/plugins
)
We still have some thinking to do with regards to the return URL, because the label for the link back is currently hardcoded as "Visit Dashboard"
so I wonder whether we'd also need a way to have that change based on the return URL!
@p-jackson I think it may be a good consensus. If we go with the idea from the third PR, we won't' need to modify routing at all and we won't increase the bundle size for all sections.
I've added some comments related to this under the PR. However, I have more thoughts about the contextual links and headings in our new Sites Dashboard, I've described them pdKhl6-vB-p2 |
Closing in favor of #65524. |
Proposed Changes
This PR replaces the current Sites component accessible by the "All My Sites" link with the new Sites Dashboard.
The feature is kept behind the
build/sites-dashboard
feature flag.Testing Instructions
Related to #65181