Enhancement/9335 SiWG Setup Banner CTA #9560
Conversation
|
Build files for 3a3caf2 have been deleted. |
|
Size Change: +5.21 kB (+0.28%) Total Size: 1.85 MB
ℹ️ View Unchanged
|
…th new notification infrastructure.
…and desktop setup CTAs.
There was a problem hiding this comment.
Thanks @jimmymadon LGTM
Although I think refactoring the description paragraph could have been better as a separate issue, since it includes additional changes, they are fairly small so I think it is fine to be included as part of the PR
There was a problem hiding this comment.
The consent mode changes in this PR are intentional, right? I don't see any notes about them in the PR description, but I'm guessing it's just some HTML element updates that were needed?
Just making sure they should be here…
There was a problem hiding this comment.
So to reuse the same <Description> component within the new layout - I had to make it similar to the way it is being used in existing layouts. This meant wrapping the p in a div. Alternatively, I could've created YET another set of unique styles for this banner compared to the others which would eventually have to be refactored in the future. So I just decided to make minor modifications to the ConsentModeSetupCTAWidget as well as AdsModuleSetupCTAWidget as these use the same description styles.
|
|
||
| return ( | ||
| <div className="googlesitekit-publisher-win__actions"> | ||
| <div className={ className }> |
There was a problem hiding this comment.
We shouldn't allow developers to replace the existing class name, I'd think… so we should do this instead:
| <div className={ className }> | |
| <div className={ classnames( 'googlesitekit-publisher-win__actions', className ) }> |
There was a problem hiding this comment.
I understand, but here, the class name is essentially a flag for the version we need to use for this component. Unfortunately, googlesitekit-publisher-win__actions and googlesitekit-setup-cta-banner__actions-wrapper are conflicting with each other - so we can either apply one or the other. If we were to use classnames, we would have to create another flag like type or something solely for styling. Type could then be banner-notification or setup-cta. And then we'd have to switch the classname based on the type. Ideally, we should be refactoring the legacy banner notifications (viz. all classes with googlesitekit-publisher-win*) soon.
So is this worth add the whole type flag, etc or should I leave this as is?
There was a problem hiding this comment.
I guess in that case we can leave it as-is, fair enough.
| return ( | ||
| <div className="googlesitekit-publisher-win__desc"> | ||
| <div className={ className }> |
There was a problem hiding this comment.
Same here; let's use classnames()
There was a problem hiding this comment.
Same reasoning as above.
| title, | ||
| description, | ||
| actions, | ||
| SVG, |
There was a problem hiding this comment.
Capital prop names are a bit odd. I'm guessing it's done so you can use <SVG /> below…
Looks like we do this elsewhere too, so no need to change it, but I guess leaving my comment here so it's recognised 😆
There was a problem hiding this comment.
Hmm, this just clearly screams that we need a component as a prop here - similar to how we use pass Widget, WidgetNull, Notification, etc as well.
| @@ -0,0 +1,37 @@ | |||
| /** | |||
| * Sign In With Google Setup CTA Banner styles. | |||
There was a problem hiding this comment.
| * Sign In With Google Setup CTA Banner styles. | |
| * Sign in with Google Setup CTA Banner styles. |
There was a problem hiding this comment.
@tofumatt Just to clarify, these changes are actually standardising the fonts of the description for existing components on certain viewports.
|
Thanks for the explanations/clarifications, this is good-to-go 👍🏻 |
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