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
Split the Global Styles variations in two: default and premium #77333
Conversation
Here is how your PR affects size of JS and CSS bundles shipped to the user's browser: App Entrypoints (~1 bytes removed 📉 [gzipped])
Common code that is always downloaded and parsed every time the app is loaded, no matter which route is used. Sections (~110 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 (~69 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. |
With these props, we'll be able to display the style variations for different theme types.
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.
Nice job, this is quite a big improvement! It works well, but I think there are a few things we need to address first.
I think this breaks the "Premium" badge we display on the thumbnails when selecting a custom variation:
Before | After |
---|---|
This text feels too wordy, is it really necessary? The original mockup didn't include it:
Mockup | This PR |
---|---|
Seems the tiles are a bit misaligned with the text?
Mockup | This PR |
---|---|
The font size in the sidebar feels smaller when compared to production and I think I like it more how it was before. Can we keep using the same font size?
Before | After |
---|---|
Nitpick, non-blocking: On mobile, when the site is on a paid plan, the "Styles" header seems unnecessary.
Before | After |
---|---|
In the theme showcase, when the site is on a paid plan, there is a huge separation between the header and the styles grid. We can probably reduce it:
Before | After |
---|---|
In the mobile view of the theme showcase, when the site is on a Free plan, I think the header takes a considerable amount of space. Can we maybe reuse the same layout that we use in the design picker, so all the styles are displayed in the same horizontal line?
Before | After |
---|---|
I think it's better to keep the existing split mode because it's more clear and we describe better what style is free and what styles are paid. In the design picker I used that approach because there's enough space to use the same design, so had to use that approach. |
These two items remain unchanged, but didn't get your thoughts on them. Did you miss them?
|
Strange, they should have been fixed. 🤔
Fair enough. Maybe I can add the text at the bottom of the UI. :) |
I've just checked again and I don't get that subheader text in the Theme showcase and Design picker. 🤔 |
…wports in Theme showcase and set the item size on mobile view ports to 80px in the theme showcase.
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.
premiumBadge={ | ||
showGlobalStylesPremiumBadge && ( | ||
<PremiumBadge | ||
className="design-picker__premium-badge" |
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.
This class is no longer used, so I think we can remove a few styles from the SCSS file
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.
Oh that's right. I planned to remove it but I guess I forgot about it.
packages/global-styles/src/components/global-styles-variations/index.tsx
Outdated
Show resolved
Hide resolved
<div className="theme__sheet-style-variations-previews"> | ||
<AsyncLoad | ||
require="@automattic/design-preview/src/components/style-variation" |
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 this component is no longer used, can we remove it?
…/index.tsx Co-authored-by: Miguel Torres <1233880+mmtr@users.noreply.github.com>
I've updated the description with some up to date screenshots. Hey @lucasmendes-design can you also have a look at the proposed changes, especially on the mobile viewports? The changes were implemented based on the desktop design and based on the existing UI on mobile viewports. |
Hi folks, some notes about the experience here:
|
This Pull Request is now available for translation here: https://translate.wordpress.com/deliverables/8021142 Thank you @BogdanUngureanu for including a screenshot in the description! This is really helpful for our translators. |
Translation for this Pull Request has now been finished. |
Related to https://github.com/Automattic/dotcom-forge/issues/2261, https://github.com/Automattic/dotcom-forge/issues/2260
Proposed Changes
Testing Instructions
site-setup/designSetup?siteSlug=your-site
(where the your-site is free)Screenshots
Theme showcase
Free site, free theme
Premium site, free theme
Free site, free theme (mobile)
Design Picker
Free site, free theme
Free site, free theme (mobile)
Pre-merge Checklist