-
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
Limit Global Styles: Add premium badge to style selection during onboarding #69082
Conversation
Here is how your PR affects size of JS and CSS bundles shipped to the user's browser: App Entrypoints (~204 bytes added 📈 [gzipped])
Common code that is always downloaded and parsed every time the app is loaded, no matter which route is used. Sections (~67 bytes added 📈 [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 (~100 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. |
Nice work, Miguel! Works great on desktop, however on mobile I've noticed some small glitches while testing it on my phone:
|
@mmtr it's possible to get to the style selection with an upgraded site already; in that case, we shouldn't show the Premium badge. How do you feel about creating some sort of |
…styles-onboarding-notice
I don't think I can fix that, since the preview below has a Apparently the So, we either (1) keep the current behavior that only show/hide the tooltips upon clicking on them or (2) completely disable the tooltips on the mobile viewport. I personally prefer the first option.
For some reason I don't understand, the same |
…styles-onboarding-notice
return { | ||
shouldLimitGlobalStyles, | ||
}; | ||
} |
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 wonder if /landing/stepper
is the right place for this hook, but at the same time, I don't see a more generic location that would be a good fit for it. 🤔
(Not a blocker, just a consideration.)
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.
Yeah, I wondered about that too and initially added it to the calypso/lib
path, but it felt a bit off so I finally moved it to where most of these generic hooks are.
This Pull Request is now available for translation here: https://translate.wordpress.com/deliverables/7583529 Thank you @mmtr for including a screenshot in the description! This is really helpful for our translators. |
Translation for this Pull Request has now been finished. |
Fixes https://github.com/Automattic/dotcom-forge/issues/896
Proposed Changes
Adds the premium badge to the style selection during onboarding.
To achieve this, I decided to slightly refactor the premium badge component introduced by #68841 to better support this use case:
components
folder.Testing Instructions
/start
and create a new free site/start
again and create a new paid site now