Enhancement/9887 Refactor Consent Mode Setup Widget #9986
Conversation
|
Build files for a5f00fe are ready:
|
|
Size Change: +4.71 kB (+0.24%) Total Size: 1.99 MB
ℹ️ View Unchanged
|
There was a problem hiding this comment.
Thanks @jimmymadon ! A few things to consider or address here, let me know if you have any questions. Great to see the main dashboard being cleaned up!
assets/js/modules/ads/index.js
Outdated
| // The getSetupErrorMessage selector relies on the resolution | ||
| // of the getSiteInfo() resolver. | ||
| await resolveSelect( CORE_SITE ).getConsentModeSettings(); |
There was a problem hiding this comment.
The comment doesn't make sense here. Copy/paste mistake?
| @@ -64,6 +64,7 @@ Default.decorators = [ | |||
| }, | |||
| ]; | |||
| Default.scenario = { | |||
| // eslint-disable-next-line sitekit/no-storybook-scenario-label | |||
There was a problem hiding this comment.
This shouldn't be necessary since no changes were made to the file 🤔
We should really avoid adding these ignores here and where needed, update the Story or scenario to comply with our convention instead.
There was a problem hiding this comment.
So for some reason, when I merged develop into my branch and tried to commit the merge, I was forced to do this otherwise the github hooks wouldn't let me commit. Since this issue has gone above estimate already, I didn't want to spend too much time on sorting these out and wanted to create another issue to do this.
There was a problem hiding this comment.
I thought skipping the pre-commit hooks would cause issues in the CI checks. But I think this is fine. Have reverted them all.
| gaTrackingEventArgs: PropTypes.shape( { | ||
| category: PropTypes.string, | ||
| label: PropTypes.string, | ||
| value: PropTypes.string, |
There was a problem hiding this comment.
This seems to be okay for now, so long as these are passed as positional args to trackEvent but I'd like to move to a more modern application where we call it as a wrapper for gtag such as trackEvent(eventName, args) => gtag('event', eventName, args). The difference being that in that paradigm, category would be event_category (the name of the dimension, same with label`. See
site-kit-wp/assets/js/util/tracking/createTrackEvent.js
Lines 49 to 51 in 37b72fa
This is something I've been meaning to open an issue to do.
There was a problem hiding this comment.
This definitely would make things more obvious.
| { learnMoreLink } | ||
| </p> | ||
| </div> | ||
| { errorText && <ErrorText message={ errorText } /> } |
There was a problem hiding this comment.
Feels like this component is starting to do a bit too much maybe.
There was a problem hiding this comment.
I did consider creating a separate component but didn't because the <ErrorText> component already encapsulates the rendering of the error component which we forward a very simple message to for now. And adding it to adds errorText support for all layouts rather than adding a new component/prop to every layout that might require it.
| <div className="googlesitekit-widget-context"> | ||
| <Grid className="googlesitekit-widget-area"> |
There was a problem hiding this comment.
This looks quite hackish – why are doing this especially just for a single notification?
Edit: I see now that this is done in other notification components as well, which I don't understand. We shouldn't emulating this in new notifications, but maybe it's only intended to be temporary? If not, let's plan on refactoring this out in a future issue.
There was a problem hiding this comment.
This started off when we "copy-pasted" the first setup CTA (I think Audience Segmentation or Consent Mode) and this has been copy-pasted since in all Setup Banners that are "separate" like widgets and not "attached" to the Navigation bar like our other Banner Notifications. We made a conscious decision not to spend too much time on restyling everything as this was part of a future phase and not in scope for this epic.
This will definitely all be restyled in BNR Phase 3 - I will be adding these notes to the Design Doc and future issues.
| @media (min-width: $width-tablet + px) { | ||
| padding: $grid-gap-desktop; | ||
| } | ||
| .googlesitekit-setup-cta-banner__svg-wrapper--consent-mode-setup-cta-widget { |
There was a problem hiding this comment.
-- should indicate a variant or state. Do we really need a special variant just for consent mode?
There was a problem hiding this comment.
All the SVGs within notifications are really "non-standard" as they are "borderless" in different areas and resizing them is a pain. This is something I want to raise with Sigal when defining the new notifications. So each of the SVGs have some different styles to make them "work". We'd be better off having standard "side-kick" like images which are fixed in size/proportion like the other older SVGs we use in our notifications.
| <a | ||
| aria-label="Learn more about consent mode (opens in a new tab)" |
There was a problem hiding this comment.
Part of the aria-label is lost here. Is this expected?
There was a problem hiding this comment.
I did think it wasn't worth passing more props here again and to just use the default standard label that Link generates. The extra "about consent mode" doesn't really add much value here. But I've added this now as it might help in the future too for more custom aria labels.
| { ! viewOnlyDashboard && ( | ||
| <Fragment> | ||
| <ConsentModeSetupCTAWidget /> | ||
| </Fragment> | ||
| ) } | ||
|
|
There was a problem hiding this comment.
Yup - I had the same amount of pleasure in getting rid of this finally! And we've done the same with ErrorNotifications as well.
assets/js/util/type-scale.stories.js
Outdated
| @@ -115,6 +115,7 @@ function Template() { | |||
|
|
|||
| export const DefaultTypeScale = Template.bind( {} ); | |||
| DefaultTypeScale.scenario = { | |||
| // eslint-disable-next-line sitekit/no-storybook-scenario-label | |||
There was a problem hiding this comment.
Please remove these added ignores
Summary
Addresses issue:
ConsentModeSetupCTAWidgetto not display simultaneously with other CTAs #9887Relevant 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