Skip to content
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

Add the Info Notice (Storybook) #8137

Closed
7 tasks
techanvil opened this issue Jan 24, 2024 · 9 comments
Closed
7 tasks

Add the Info Notice (Storybook) #8137

techanvil opened this issue Jan 24, 2024 · 9 comments
Labels
Module: Analytics Google Analytics module related issues P1 Medium priority Type: Enhancement Improvement of an existing feature

Comments

@techanvil
Copy link
Collaborator

techanvil commented Jan 24, 2024

Feature Description

Create the Info Notice widget component and add it to Storybook. It should display a static message, the full message sequence will be added separately via #8139.

See info notice in the design doc.


Do not alter or remove anything below. The following sections will be managed by moderators only.

Acceptance criteria

  • The InfoNotice component should be added along with its stories and styled as per the designs.
  • It should have at least the following properties:
    • icon
    • notice
    • ctaLabel
    • ctaOnClick

Implementation Brief

  • Add assets/js/modules/analytics-4/components/common/AudienceSegmentation/infoNotice.js
    • Define following props:
      • content - Main text, it should be output within p tag
      • dismissLabel - A CTA label
      • onDismiss - Optional callback that will be invoked when button is clicked
    • Wrap the output in main div with class, say googlesitekit-audience-segmentation-info-notice
    • Include the SVG icon from figma
    • Add Button component with tertiary prop, and use dismissLabel for button text
      • onClick should invoke a passed onDismiss callback function
  • Add assets/sass/components/analytics-4/audience-segmentation/info-notice.scss and apply styling to match the figma design

Test Coverage

  • Add tests and stories for InfoNotice component. Use the content from referenced figma design for the component in stories

QA Brief

Changelog entry

  • Add the Audience Segmentation Info Notice as a component in Storybook.
@techanvil techanvil added Module: Analytics Google Analytics module related issues P1 Medium priority Type: Enhancement Improvement of an existing feature labels Jan 24, 2024
@ivonac4 ivonac4 added the Next Up Issues to prioritize for definition label Feb 5, 2024
@asvinb asvinb assigned asvinb and unassigned asvinb Feb 8, 2024
@eugene-manuilov eugene-manuilov self-assigned this Feb 8, 2024
@eugene-manuilov
Copy link
Collaborator

AC ✔️

@eugene-manuilov eugene-manuilov removed their assignment Feb 8, 2024
@zutigrm zutigrm assigned zutigrm and unassigned zutigrm Feb 9, 2024
@eugene-manuilov eugene-manuilov self-assigned this Feb 9, 2024
@eugene-manuilov
Copy link
Collaborator

Thanks, @zutigrm. This is a good start but I think we need to improve IB a little bit. The main problem from my point of view is that IB tells us to implement everything within the InfoNotice component (btw you need to update the file name to be InfoNotice.js instead of infoNotice.js in your IB) making it responsible for determining whether we need to render it or not, what to render and handle dismissals. This is too much for that component.

Let's introduce one more component InfoNotices that will be responsible for business logic and leave InfoNotice to render what is passed into it via props. In this case, these components will be easier to maintain and each of them will do only one thing. WDYT?

@zutigrm
Copy link
Collaborator

zutigrm commented Feb 13, 2024

Thanks @eugene-manuilov for the feedback

btw you need to update the file name to be InfoNotice.js instead of infoNotice.js in your IB

Oh indeed, good catch, thanks!

The main problem from my point of view is that IB tells us to implement everything within the InfoNotice component

Let's introduce one more component InfoNotices that will be responsible for business logic and leave InfoNotice to render what is passed into it via props. In this case, these components will be easier to maintain and each of them will do only one thing. WDYT?

That's a good point. Although regarding InfoNotices component, it would imply that it holds multiple notices, and it would be expected to accept some kind of an array as props. What do you think if we let parent component that will include InfoNotice to handle dismissal callback and rendering conditionals instead of making InfoNotices. What do you think?

@eugene-manuilov
Copy link
Collaborator

What do you think if we let parent component that will include InfoNotice to handle dismissal callback and rendering conditionals instead of making InfoNotices

This will add an extra responsibility to the parent component. Ideally, we should try to follow the single responsibility principle if we can. What if we use another name for that component? For example, instead of InfoNotices we use InfoNoticeDispatcher? In this case, it won't sound like it holds multiple notices, it will sounds like it controls info notices and makes a decision when to display it. WDYT?

@eugene-manuilov eugene-manuilov removed their assignment Feb 13, 2024
@zutigrm
Copy link
Collaborator

zutigrm commented Feb 14, 2024

@eugene-manuilov That makes sense, thanks for the pointer. I will update IB to propose creation of InfoNoticeDispatcher

@zutigrm zutigrm assigned eugene-manuilov and unassigned zutigrm Feb 14, 2024
@eugene-manuilov
Copy link
Collaborator

Thanks, @zutigrm. IB looks better now, but after re-reading this task it seems that we don't need InfoNoticeDispatcher component yet 🤔. Looks like just the InfoNotice component that renders content provided via props is sufficient for this task. Sorry about this, but let's probably drop InfoNoticeDispatcher from IB as it will be implemented somewhere else.

@zutigrm
Copy link
Collaborator

zutigrm commented Feb 14, 2024

@eugene-manuilov Got it, IB updated

@zutigrm zutigrm assigned eugene-manuilov and unassigned zutigrm Feb 14, 2024
@eugene-manuilov
Copy link
Collaborator

Thanks, @zutigrm. IB 🌶️

@eugene-manuilov eugene-manuilov removed their assignment Feb 15, 2024
@ivonac4 ivonac4 removed the Next Up Issues to prioritize for definition label Feb 16, 2024
@derweili derweili self-assigned this Mar 12, 2024
@derweili derweili removed their assignment Mar 14, 2024
@techanvil techanvil assigned techanvil and derweili and unassigned techanvil Mar 14, 2024
@derweili derweili assigned techanvil and unassigned derweili Mar 15, 2024
@techanvil techanvil removed their assignment Mar 15, 2024
@mohitwp mohitwp self-assigned this Mar 15, 2024
@mohitwp
Copy link
Collaborator

mohitwp commented Mar 20, 2024

QA Update ✅

  • Verified on storybook dev environment.
  • Verified that info notice is appearing is same as Figma design.
  • Note : CTA font color is different in Figma and we implemented the CTA color which we are using globally on SK.
  • Verfied info notice have icon, notice, ctaLabel and ctaOnClick.

image

image

@mohitwp mohitwp removed their assignment Mar 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Module: Analytics Google Analytics module related issues P1 Medium priority Type: Enhancement Improvement of an existing feature
Projects
None yet
Development

No branches or pull requests

8 participants