Sites: Cleanup unused props from SiteSelector
#102382
Conversation
tyxla
added
Sites
[Status] Needs Review
Jetpack Cloud live (direct link)
Automattic for Agencies live (direct link)
|
|
Here is how your PR affects size of JS and CSS bundles shipped to the user's browser: App Entrypoints (~2398 bytes added 📈 [gzipped]) DetailsCommon code that is always downloaded and parsed every time the app is loaded, no matter which route is used. Sections (~35744 bytes added 📈 [gzipped]) DetailsSections contain code specific for a given set of routes. Is downloaded and parsed only when a particular route is navigated to. Async-loaded Components (~32525 bytes added 📈 [gzipped]) DetailsReact 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. |
|
This PR modifies the release build for the following Calypso Apps: For info about this notification, see here: PCYsg-OT6-p2
To test WordPress.com changes, run |
Isn't this an unintentional bug in CSS styles of the site picker? The "Add new site" button is part of a vertically laid-out flexbox container, and due to the styles ( 
Clicking the button leads to |
There was a problem hiding this comment.
No regressions spotted 👍
Two unrelated issues found in testing: #102390, #101831
The "Add new site" button is part of a vertically laid-out flexbox container, and due to the styles (
flex-grow,flex-shrink, etc) it's never displayed.
Good catch. I cannot detect any intentionality in this being hidden, nor could I pinpoint where exactly this regressed. For example #67459 and #67295 are where the flex: 1 and magin-top: auto rules were added, but based on the screenshots it was still working as expected.
Do you know who to ping? Bugomattic is not helping me here and in any case who knows right now 😅 Otherwise we can post an issue.
| selected: PropTypes.oneOfType( [ PropTypes.number, PropTypes.string ] ), | ||
| hideSelected: PropTypes.bool, | ||
| filter: PropTypes.func, | ||
| groups: PropTypes.bool, |
There was a problem hiding this comment.
I guess we can also remove these if the prop was noop-ing?
diff --git a/client/blocks/reader-share/reblog.jsx b/client/blocks/reader-share/reblog.jsx
index f0fa3820c87..811c345a841 100644
--- a/client/blocks/reader-share/reblog.jsx
+++ b/client/blocks/reader-share/reblog.jsx
@@ -49,11 +49,7 @@ const ReaderReblogSelection = ( props ) => {
popoverTitle={ translate( 'Repost on' ) }
onClose={ props.closeMenu }
>
- <SiteSelector
- className="reader-share__site-selector"
- onSiteSelect={ pickSiteToShareTo }
- groups
- />
+ <SiteSelector className="reader-share__site-selector" onSiteSelect={ pickSiteToShareTo } />
</ReaderPopoverMenu>
);
};
diff --git a/client/my-sites/sites/index.jsx b/client/my-sites/sites/index.jsx
index 9c5a2d08e4c..2a6031cd3c1 100644
--- a/client/my-sites/sites/index.jsx
+++ b/client/my-sites/sites/index.jsx
@@ -254,7 +254,7 @@ class Sites extends Component {
{ fromSite && <VisitSite siteSlug={ fromSite } /> }
</div>
<Card className="sites__select-wrapper">
- <SiteSelector filter={ this.filterSites } siteBasePath={ siteBasePath } groups />
+ <SiteSelector filter={ this.filterSites } siteBasePath={ siteBasePath } />
</Card>
</>
) }
My gut tells me it's not intentional, and should be restored — should we check with product/designers? (@keoshi ) |
Yep, I agree, it might be the case that it's unintentional, so let's double-check. Also cc the rest of @Automattic/jetpack-design for feedback |
Re-sending the ping in case the earlier one went unnoticed — @Automattic/jetpack-design @keoshi |
|
@ciampo I think this should evenutally be closed/revisited, which is why I added the "blocked" / "do not merge" labels. I think we should consider using the "add new button" that we use everywhere, and this will obsolete this old one in the sidebar. Keeping this PR open in the meantime, and I'd like to get back to it, but I haven't been able to work on it in due to other commitments. |
|
Sorry, folks. Completely missed this in my inbox. I can't remember the button ever being there, but it should totally be visible. Not sure if a complete oversight or a bug introduced long ago. Unrelated but the flow after that also has some bugs. Notably, the |
|
This PR has been marked as stale. This happened because:
If this PR is still useful, please do a trunk merge or rebase and otherwise make sure it's up to date and has clear testing instructions. You may also want to ping possible reviewers in case they've forgotten about it. Please close this PR if you think it's not valid anymore — if you do, please add a brief explanation. If the PR is not updated (or at least commented on) in another month, it will be automatically closed. |
|
Going to close for now, all this work has to be revisited at some point as Calypso is being untangled and consolidated with the hosting dashboard project. |
github-actions
bot
removed
[Status] Needs Review
Proposed Changes
This PR suggests cleaning up a few more unused props and related code from the
SiteSelectorcomponent, specifically:showManageSitesButton: was not in use after Sites: Remove legacy site picker #102364 landed.showAddNewSite: was used, but the related UI was never visible (see below screenshots).groups: was not used at all.Could be more straightforward to review on a per-commit basis.
Why are these changes being made?
Just cleaning up unused code.
A follow-up to #102364 that did some more cleanup.
Lands part of a larger cleanup in #101844.
Testing Instructions
Screenshots
Jetpack Cloud, single user account, site selection:
Jetpack Cloud, partner account, site selection:
