Implement the RRM express setup dashboard entry point.#13107
Conversation
🎭 Playwright reports for af5343b: 📚 Storybook for af5343b: 📦 Build files for af5343b:
|
|
Failing E2E tests are the usual suspects and VRT seems unrelated and passes locally. |
|
Size Change: +12.5 kB (+0.34%) Total Size: 3.72 MB 📦 View Changed
ℹ️ View Unchanged
|
| padding-top: 0; | ||
| } | ||
|
|
||
| .googlesitekit-banner--rrm-setup { |
There was a problem hiding this comment.
I styled the banner in here with this class, rather than what's described in the IB, as it felt like more idiomatic BEM to me, being a variation of a banner.
There was a problem hiding this comment.
I'd argue this is a consumer of Banner rather than a variation, so the class name should ideally be along the lines of googlesitekit-rrm-cta-banner, and thus, these styles should be relocated to its own stylesheet.
There was a problem hiding this comment.
@nfmohit Have to say I really disagree with this. The banner is intimately tied to .googlesitekit-banner component because it needs to style the googlesitekit-banner__ elements anyway, and the styles are useless without the original component styles. In my mind it's a pretty textbook case for a BEM modifier class.
There was a problem hiding this comment.
I get the BEM angle, but I still see this as a consumer of Banner rather than a Banner variation.
--setup-cta makes sense as a shared modifier because it’s reused across several setup CTAs and only tweaks the shared look. What’s in --rrm-setup is RRM-specific (gradient, typography, spacing, widget border-radius) and isn’t meant to be reused elsewhere, so I’d keep it out of the Banner stylesheet.
We’ve done this before with feature-specific Banner consumers. e.g. ConnectGA4CTAWidget uses googlesitekit-banner--setup-cta for the shared bits, then googlesitekit-km-connect-ga4-cta for the one-off overrides, with those styles living in _googlesitekit-key-metrics-setup-cta.scss (still targeting .googlesitekit-banner__* from there).
I do acknowledge your reasoning; your approach is more strictly BEM, but I’d rather stick with how we usually handle this in Site Kit, i.e., feature-specific overrides living with the feature under its own class.
So, I’d lean toward something like googlesitekit-rrm-cta-banner, even if it reaches into Banner’s elements. Happy to discuss further if you feel strongly otherwise.
There was a problem hiding this comment.
@nfmohit I've moved the styles into a .googlesitekit-rrm-setup-cta-banner class in its own file in the module. It required an ugly &.googlesitekit-banner { to fix specificity issues that it caused, which is another reason I'm not a big fan, but I understand the need for consistency and feature ownership reasoning.
|
|
||
| @media (min-width: $width-tablet + 1 + px) { | ||
| p.googlesitekit-banner__title { | ||
| @include googlesitekit-typography(headline, small); |
There was a problem hiding this comment.
I thought a mixin like this would be useful for re-using typography styles on elements that aren't using the Typography component, like the banner heading, and need to switch between styles at different breakpoints.
There was a problem hiding this comment.
This looks clean, fantastic work!
Applying this mixin means overriding more styles than we need to, because the Banner component by default matches most of the Figma designs, but it does look much cleaner, easier to read, and more referable to the Figma designs.
| RRM_EXPRESS_SETUP_TRAFFIC_CTA_WIDGET_SLUG, | ||
| } from '@/js/modules/reader-revenue-manager/constants'; | ||
|
|
||
| // @ts-expect-error TODO: Add type for `widgets` when googlesitekit-widgets is migrated to TypeScript. |
| padding-top: 0; | ||
| } | ||
|
|
||
| .googlesitekit-banner--rrm-setup { |
There was a problem hiding this comment.
I'd argue this is a consumer of Banner rather than a variation, so the class name should ideally be along the lines of googlesitekit-rrm-cta-banner, and thus, these styles should be relocated to its own stylesheet.
|
|
||
| @media (min-width: $width-tablet + 1 + px) { | ||
| p.googlesitekit-banner__title { | ||
| @include googlesitekit-typography(headline, small); |
There was a problem hiding this comment.
This looks clean, fantastic work!
Applying this mixin means overriding more styles than we need to, because the Banner component by default matches most of the Figma designs, but it does look much cleaner, easier to read, and more referable to the Figma designs.
| } | ||
| } | ||
|
|
||
| @media (min-width: $width-tablet + 1 + px) { |
There was a problem hiding this comment.
Why don't we use $bp-tablet here?
There was a problem hiding this comment.
@nfmohit I was just matching the media queries used for the styles being overwritten, but I can update both to $bp-nonTablet, which includes the 1px.
There was a problem hiding this comment.
@nfmohit I've replaced $width-tablet + 1 + px with $bp-nonMobile and $width-desktop + 1 + px with $bp-nonTablet throughout this file.
There was a problem hiding this comment.
Thanks! I didn't realize that this stylesheet was already using the width + 1 pattern, so we could've stuck with that too. Either is fine in that case, thank you!
| } | ||
| } | ||
|
|
||
| @media (min-width: $width-desktop + 1 + px) { |
There was a problem hiding this comment.
Why don't we use $bp-desktop here?
There was a problem hiding this comment.
@nfmohit I was just matching the media queries used for the styles being overwritten, but I can update both to $bp-nonTablet, which includes the 1px.
There was a problem hiding this comment.
@nfmohit I've replaced $width-tablet + 1 + px with $bp-nonMobile and $width-desktop + 1 + px with $bp-nonTablet throughout this file.
There was a problem hiding this comment.
Thanks! I didn't realize that this stylesheet was already using the width + 1 pattern, so we could've stuck with that too. Either is fine in that case, thank you!
Co-authored-by: Nahid F Mohit <nahid.mohit@10up.com>
| export const Default = Template.bind( {} ); | ||
| Default.storyName = 'Default'; | ||
| Default.scenario = {}; |
There was a problem hiding this comment.
Could we fix the type errors here? We'd have to declare a type for Default.
| export const Default = Template.bind( {} ); | |
| Default.storyName = 'Default'; | |
| Default.scenario = {}; | |
| export const Default = Template.bind( {} ) as Story< WidgetComponentProps >; | |
| Default.storyName = 'Default'; | |
| Default.scenario = {}; |
|
|
||
| export default function PoweredByReaderRevenueManager() { | ||
| return ( | ||
| <div className="googlesitekit-powered-by googlesitekit-powered-by--reader-revenue-manager"> |
There was a problem hiding this comment.
Looks okay to me. There is indeed a name property if you use the getModule( slug ) selector. See:
If we go with the module definition approach, we should probably rename the component to PoweredByModule or something along those lines, WDYT?
There was a problem hiding this comment.
Let's use TS for this file too please.
| } | ||
| } | ||
|
|
||
| @media (min-width: $width-tablet + 1 + px) { |
There was a problem hiding this comment.
Thanks! I didn't realize that this stylesheet was already using the width + 1 pattern, so we could've stuck with that too. Either is fine in that case, thank you!
| } | ||
| } | ||
|
|
||
| @media (min-width: $width-desktop + 1 + px) { |
There was a problem hiding this comment.
Thanks! I didn't realize that this stylesheet was already using the width + 1 pattern, so we could've stuck with that too. Either is fine in that case, thank you!
| padding-top: 0; | ||
| } | ||
|
|
||
| .googlesitekit-banner--rrm-setup { |
There was a problem hiding this comment.
I get the BEM angle, but I still see this as a consumer of Banner rather than a Banner variation.
--setup-cta makes sense as a shared modifier because it’s reused across several setup CTAs and only tweaks the shared look. What’s in --rrm-setup is RRM-specific (gradient, typography, spacing, widget border-radius) and isn’t meant to be reused elsewhere, so I’d keep it out of the Banner stylesheet.
We’ve done this before with feature-specific Banner consumers. e.g. ConnectGA4CTAWidget uses googlesitekit-banner--setup-cta for the shared bits, then googlesitekit-km-connect-ga4-cta for the one-off overrides, with those styles living in _googlesitekit-key-metrics-setup-cta.scss (still targeting .googlesitekit-banner__* from there).
I do acknowledge your reasoning; your approach is more strictly BEM, but I’d rather stick with how we usually handle this in Site Kit, i.e., feature-specific overrides living with the feature under its own class.
So, I’d lean toward something like googlesitekit-rrm-cta-banner, even if it reaches into Banner’s elements. Happy to discuss further if you feel strongly otherwise.
Co-authored-by: Nahid F Mohit <nahid.mohit@10up.com>
Co-authored-by: Nahid F Mohit <nahid.mohit@10up.com>
Co-authored-by: Nahid F Mohit <nahid.mohit@10up.com>
Co-authored-by: Nahid F Mohit <nahid.mohit@10up.com>
|
@nfmohit GitHub's threads are becoming unwieldy but I think I've responded to or fixed everything. |
2 participants
Summary
Addresses issue:
Relevant technical choices
PR Author Checklist
Do not alter or remove anything below. The following sections will be managed by moderators only.
Code Reviewer Checklist
Merge Reviewer Checklist