-
Notifications
You must be signed in to change notification settings - Fork 2k
Plugins redesign: Add new copy and redesign styling for the details view #107463
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
Conversation
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: Sections (~415 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. 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. |
2bcffda to
233a14f
Compare
d8b2a4d to
fa25717
Compare
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.
Pull request overview
This PR implements styling redesign changes for the plugin details page as part of the marketplace redesign initiative. The changes are conditionally applied when the marketplace-redesign feature flag is enabled, ensuring backward compatibility.
Key Changes:
- Added full-width layout support with new styling for plugin details page components
- Implemented a carousel-based display for related plugins with responsive breakpoints
- Restructured review cards UI with updated spacing and container wrappers
- Consolidated shared marketplace footer styles into a common location
Reviewed changes
Copilot reviewed 15 out of 15 changed files in this pull request and generated 17 comments.
Show a summary per file
| File | Description |
|---|---|
| client/my-sites/plugins/style.scss | Added full-width section styles, consolidated marketplace footer styles, and updated navigation/layout padding |
| client/my-sites/plugins/related-plugins/style.scss | Added carousel-specific styles and responsive grid layouts for related plugins |
| client/my-sites/plugins/related-plugins/index.tsx | Implemented carousel functionality with DotPager, chunking logic, and conditional rendering |
| client/my-sites/plugins/plugins-browser/style.scss | Updated search header padding and removed duplicate marketplace footer styles |
| client/my-sites/plugins/plugins-browser/index.jsx | Updated class name reference for marketplace footer |
| client/my-sites/plugins/plugin-details.jsx | Restructured layout with FullWidthSection wrappers and fixed typo in error message |
| client/my-sites/plugins/plugin-details-sidebar/style.scss | Added text-transform for title in full-width layout |
| client/my-sites/plugins/plugin-details-header/style.scss | Updated header sizing, icon dimensions, and rating display styles |
| client/my-sites/plugins/plugin-details-header/index.jsx | Added Rating component integration and conditional rendering based on feature flag |
| client/my-sites/plugins/plugin-details-CTA/style.scss | Updated font sizes for full-width section |
| client/my-sites/plugins/education-footer/index.tsx | Hidden decorative element in full-width layout |
| client/my-sites/marketplace/components/reviews-cards/*.tsx | Wrapped components in FullWidthSection and updated card structure |
| client/components/rating/style.scss | Added full-width section color overrides for star ratings |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
client/my-sites/plugins/style.scss
Outdated
| body .is-section-plugins:has(.plugins-browser.is-full-width-layout.is-logged-out) .layout__content{ | ||
| padding: 0; | ||
| } | ||
| body .is-section-plugins:has(.is-plugin-details.is-full-width-layout) .layout__content{ |
Copilot
AI
Dec 4, 2025
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 selector also appears to have the same issue: body .is-section-plugins:has(...) should likely be body.is-section-plugins:has(...) for proper targeting.
| body .is-section-plugins:has(.plugins-browser.is-full-width-layout.is-logged-out) .layout__content{ | |
| padding: 0; | |
| } | |
| body .is-section-plugins:has(.is-plugin-details.is-full-width-layout) .layout__content{ | |
| body.is-section-plugins:has(.plugins-browser.is-full-width-layout.is-logged-out) .layout__content{ | |
| padding: 0; | |
| } | |
| body.is-section-plugins:has(.is-plugin-details.is-full-width-layout) .layout__content{ |
| gap: 8px; | ||
|
|
||
| .rating .gridicon { | ||
| fill: #ffb514; |
Copilot
AI
Dec 4, 2025
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.
The color value #ffb514 is hardcoded and duplicated (also in rating/style.scss line 24 as #FFB514). These should reference the same shared variable for consistency. Note the inconsistent casing between the two occurrences.
| <> | ||
| <div>{ `${ formatPluginRating( rating, true ) }/5` }</div> | ||
| </> |
Copilot
AI
Dec 4, 2025
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.
Unnecessary empty fragment. The fragment <>...</> on lines 154-156 contains only a single <div>, so the fragment wrapper is redundant and can be removed.
| <> | |
| <div>{ `${ formatPluginRating( rating, true ) }/5` }</div> | |
| </> | |
| <div>{ `${ formatPluginRating( rating, true ) }/5` }</div> |
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.
Pull request overview
Copilot reviewed 16 out of 16 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
client/my-sites/plugins/style.scss
Outdated
| body.is-section-plugins .is-logged-in .navigation-header { | ||
| padding-block: 16px; | ||
|
|
||
| &.full-width-section & { |
Copilot
AI
Dec 4, 2025
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.
Invalid CSS selector: &.full-width-section & { has a redundant trailing &. This should likely be either &.full-width-section { or .full-width-section & { depending on the intended selector hierarchy.
| &.full-width-section & { | |
| &.full-width-section { |
| @@ -1,3 +1,5 @@ | |||
| $rating-star-color: #ffb514; | |||
Copilot
AI
Dec 4, 2025
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.
The variable $rating-star-color is defined in both this file and in client/my-sites/plugins/_variables.scss (line 2). This creates duplicate definitions with the same value. Consider importing the variable from _variables.scss or centralizing it in one location to avoid maintenance issues.
| $rating-star-color: #ffb514; | |
| @import '../../my-sites/plugins/variables'; |
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.
Pull request overview
Copilot reviewed 16 out of 16 changed files in this pull request and generated 13 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| let carouselPageSize = 6; | ||
| if ( ! isLargeOrAbove ) { | ||
| carouselPageSize = 2; | ||
| } else if ( ! isWideOrAbove ) { | ||
| carouselPageSize = 4; | ||
| } |
Copilot
AI
Dec 4, 2025
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.
These magic numbers (2, 4, 6) representing carousel page sizes should be extracted to named constants with descriptive names. Consider:
const CAROUSEL_PAGE_SIZE_MOBILE = 2;
const CAROUSEL_PAGE_SIZE_MEDIUM = 4;
const CAROUSEL_PAGE_SIZE_WIDE = 6;This improves code readability and makes it easier to adjust these values in the future.
| } | ||
| } | ||
|
|
||
|
|
Copilot
AI
Dec 4, 2025
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.
There's an extra blank line here that should be removed to maintain consistent spacing in the stylesheet.
| body.is-section-plugins:has(.is-full-width-layout) { | ||
| .plugins-browser__content-wrapper { | ||
| background-color: var(--studio-white); | ||
| } | ||
|
|
||
| .full-width-section__content { | ||
| max-width: 1220px; | ||
| padding-inline: 24px; | ||
| @media screen and ( max-width: 600px ) { | ||
| padding-inline: 16px; | ||
| } | ||
| } | ||
| .lpc-header-nav-container { | ||
| .masterbar { | ||
| background: transparent; | ||
| border-bottom: none; | ||
| height: 0; | ||
| } | ||
|
|
||
| .x-nav-item, .x-dropdown-link, .x-menu-link { | ||
| color: var(--studio-gray-100); | ||
| } | ||
| .x-dropdown-link { | ||
| &:hover, &:active, &:focus { | ||
| color: var(--studio-gray-100); | ||
| background-color: var(--studio-gray-0); | ||
| } | ||
| } | ||
| .x-nav-link__logo { | ||
| --studio-blue-50: var(--studio-gray-100); | ||
| } | ||
| } | ||
| .section-nav__mobile-header { | ||
| padding-inline: 0; | ||
| } | ||
| } |
Copilot
AI
Dec 4, 2025
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.
The selector body.is-section-plugins:has(.is-full-width-layout) is very broad and contains deeply nested selectors. This creates high CSS specificity that can be difficult to override and maintain. Consider:
- Using more specific selectors at the parent level rather than deeply nested ones
- Breaking this large block into smaller, more focused rule sets
- Using CSS custom properties for shared values like max-width (1220px appears multiple times)
| type RelatedPluginProps = { | ||
| slug: string; | ||
| size: number; | ||
| size?: number; |
Copilot
AI
Dec 4, 2025
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.
The size prop has been changed from required to optional, but there's no JSDoc comment documenting this breaking change or explaining when the default values are used. Consider adding documentation:
/**
* @param size - Optional. Number of plugins to fetch.
* Defaults to 4 without marketplace-redesign flag, 18 with flag.
*/This helps API consumers understand the behavior.
| const relatedPlugins = ! showPlaceholder && ( | ||
| <RelatedPlugins | ||
| slug={ props.pluginSlug } | ||
| seeAllLink={ `/plugins/${ props.pluginSlug }/related/${ selectedSite?.slug ?? '' }` } | ||
| /> | ||
| ); |
Copilot
AI
Dec 4, 2025
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.
The relatedPlugins variable is computed outside the return statement but is now conditionally rendered in two different locations (line 535 for old design, line 556 for new design). This creates complexity and potential for bugs. Consider:
- Moving this logic into a helper function that handles the conditional rendering internally
- Or, computing it closer to where it's used
This would improve code organization and make the conditional rendering logic clearer.
| const relatedPlugins = ! showPlaceholder && ( | |
| <RelatedPlugins | |
| slug={ props.pluginSlug } | |
| seeAllLink={ `/plugins/${ props.pluginSlug }/related/${ selectedSite?.slug ?? '' }` } | |
| /> | |
| ); | |
| // relatedPlugins variable removed; logic will be inlined at render sites. |
| } | ||
|
|
||
| .full-width-section__content { | ||
| max-width: 1220px; |
Copilot
AI
Dec 4, 2025
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.
The magic number 1220px for max-width appears multiple times in this file (lines 801, 837, 857). This should be defined as a CSS custom property or SCSS variable to ensure consistency and make updates easier. For example:
$full-width-section-max-width: 1220px;| body.is-section-plugins .is-logged-in { | ||
| $full-width-main: "main:not(.hosting-dashboard-layout):not(.hosting-dashboard-layout-column):not(.is-logged-out).plugins-browser.is-full-width-layout"; | ||
|
|
||
| #{$full-width-main} { | ||
| .plugins-browser__content-wrapper { | ||
| padding: 0; | ||
| max-width: 100%; | ||
| } | ||
|
|
||
| .full-width-section__content { | ||
| max-width: 100%; | ||
| } | ||
|
|
||
| .navigation-header__container { | ||
| max-width: 100%; | ||
| } |
Copilot
AI
Dec 4, 2025
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.
The complex selector string stored in a variable is difficult to read and maintain. Consider refactoring this by:
- Using a mixin for the full-width layout styles
- Applying the styles directly with a simpler class-based selector
- Adding comments to explain what each exclusion (
:not()) is for
This would improve code readability and make it easier to modify in the future.
| body.is-section-plugins .is-logged-in { | |
| $full-width-main: "main:not(.hosting-dashboard-layout):not(.hosting-dashboard-layout-column):not(.is-logged-out).plugins-browser.is-full-width-layout"; | |
| #{$full-width-main} { | |
| .plugins-browser__content-wrapper { | |
| padding: 0; | |
| max-width: 100%; | |
| } | |
| .full-width-section__content { | |
| max-width: 100%; | |
| } | |
| .navigation-header__container { | |
| max-width: 100%; | |
| } | |
| // Full-width main layout for plugins browser when logged in | |
| // Exclude .hosting-dashboard-layout: Not a hosting dashboard layout | |
| // Exclude .hosting-dashboard-layout-column: Not a hosting dashboard column layout | |
| // Exclude .is-logged-out: Only apply when logged in | |
| body.is-section-plugins .is-logged-in main.plugins-browser.is-full-width-layout | |
| :not(.hosting-dashboard-layout) | |
| :not(.hosting-dashboard-layout-column) | |
| :not(.is-logged-out) { | |
| @include full-width-main-layout; | |
| } | |
| // Mixin for full-width main layout styles | |
| @mixin full-width-main-layout { | |
| .plugins-browser__content-wrapper { | |
| padding: 0; | |
| max-width: 100%; | |
| } | |
| .full-width-section__content { | |
| max-width: 100%; | |
| } | |
| .navigation-header__container { | |
| max-width: 100%; |
| options?: { | ||
| enabled?: boolean; | ||
| staleTime?: number; | ||
| refetchOnMount?: boolean; | ||
| }; |
Copilot
AI
Dec 4, 2025
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.
The options prop has been changed from required to optional. Similar to the size prop, this should be documented with JSDoc to explain the change and default behavior. This is particularly important as it affects how the query is configured.
| const pluginDownload = ! showPlaceholder && | ||
| ! requestingPluginsForSites && | ||
| isWporgPluginFetched && ( | ||
| <div className="plugin-details__plugin-download"> | ||
| <div className="plugin-details__plugin-download-text"> | ||
| <span>{ downloadText }</span> | ||
| </div> | ||
| <div className="plugin-details__plugin-download-cta"> | ||
| <Button | ||
| href={ `https://downloads.wordpress.org/plugin/${ | ||
| fullPlugin?.slug || '' | ||
| }.latest-stable.zip` } | ||
| rel="nofollow" | ||
| > | ||
| { isMarketplaceRedesignEnabled && <Icon icon={ download } size={ 18 } /> } | ||
| { translate( 'Download' ) } | ||
| </Button> | ||
| </div> | ||
| <script type="application/ld+json">{ structuredData }</script> | ||
| </div> | ||
| ); |
Copilot
AI
Dec 4, 2025
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.
The pluginDownload variable is computed outside the return statement and conditionally rendered in two locations (line 548 for new design, line 551 for old design) with a complex conditional expression. This duplicates the placement logic and makes the code harder to maintain. Consider extracting this into a helper function or component that encapsulates both the conditional logic and placement behavior based on the feature flag.
|
|
||
| const DEFAULT_LIST_SIZE = 4; |
Copilot
AI
Dec 4, 2025
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.
These magic numbers should be extracted to named constants at the module level or documented with comments explaining their significance. Consider adding comments like:
const DEFAULT_LIST_SIZE = 4; // Number of related plugins shown without carousel
const CAROUSEL_LIST_SIZE = 18; // Total plugins fetched for carousel paginationThis would make the code more maintainable and explain the business logic behind these values.
| const DEFAULT_LIST_SIZE = 4; | |
| // Number of related plugins shown without carousel | |
| const DEFAULT_LIST_SIZE = 4; | |
| // Total plugins fetched for carousel pagination |
7d44d85 to
e2b7b2e
Compare
Part of DOTDEV-267
Proposed Changes
Why are these changes being made?
Testing Instructions
/plugins/wordpress-seo(with and without the?flags=marketplace-redesignparam)Pre-merge Checklist