-
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
My Home: Try updating card mapping for Support card #42752
Conversation
Here is how your PR affects size of JS and CSS bundles shipped to the user's browser: Sections (~533 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. 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. |
I generated a desktop app build using the Calypso snapshot currently fixed in
The new Support Search card shows up and works as expected while current version (5.1.1) keeps displaying the old Support card.
|
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.
Thanks @mmtr for proposing this and crafting the PR 💖
This tests well for me. Let's let @getdave pick the landing approach:
- We either take My Home: Try updating card mapping for Support card #42752 and drop D43941-code
- Or keep D43941-code and close My Home: Try updating card mapping for Support card #42752
Respectively each patch needs to land last, otherwise we'll see the card in production. Going to add DO NOT MERGE
label until folks are happy with a landing choice.
Still discussing a decision on whether to merge this in |
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.
We decided in paYJgx-GU-p2#comment-917
that this was the best route to take.
Let's land this just as soon as @retrofox has landed the tracking PR.
We also need to address the followups noted in #42744
Merging today in a few minutes... |
84108dd
to
1422cf0
Compare
👋 Thanks for proposing this solution, @mmtr - I think it's a good middle ground, given the alternatives. Heads up that desktop CI is currently offline as there's a webpack-related issue that's being currently looked at in #42898. Being as you tested this manually with a desktop build, I think this change is safe to land. |
1422cf0
to
fc35c9d
Compare
This reverts commit 3574c1b.
Changes proposed in this Pull Request
Explores the approach suggested in paYJgx-GU-p2 #comment-899 as an alternative to D43941-code.
This way, we re-use the existing
FEATURE_SUPPORT
card UID so the new Support Search card will be also used in future versions of the desktop app while keeping backwards compatibility with current and previous versions.Testing instructions
Verify the Support Search shows up for all views, including the ones used on the desktop app. See #42752 (comment) for instructions about how to build a new desktop app version that includes the new Support Search card (I already generated it and confirmed it works as expected).