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 Full Width Error Banner (Storybook) #8230

Closed
13 of 15 tasks
techanvil opened this issue Feb 6, 2024 · 9 comments
Closed
13 of 15 tasks

Add the Full Width Error Banner (Storybook) #8230

techanvil opened this issue Feb 6, 2024 · 9 comments
Labels
Module: Analytics Google Analytics module related issues P1 Medium priority Squad 2 (Team M) Issues for Squad 2 Type: Enhancement Improvement of an existing feature

Comments

@techanvil
Copy link
Collaborator

techanvil commented Feb 6, 2024

Feature Description

Create the Full Width Error Banner component and add it toStorybook.

See full width error banner in the design doc.


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

Acceptance criteria

  • The Full Width Error Banner component should be created and stories for it added to Storybook.
  • It should be implemented according to the Figma design.
  • The component should accept a list of errors as a prop.
    • If any of the errors is a permissions error, the "insufficient permissions" variant of the error notice will be shown (as mentioned in the design doc, including a Get help link and a Request access buttons.
    • Otherwise, the catch-all variant will be shown which just includes the Retry button.
      • Clicking the Retry button will retry whatever triggered the errors (i.e. it will invalidate the resolution for the selector associated with the errors).

Implementation Brief

In the assets/js/modules/analytics-4/components/dashboard/AudienceSegmentation directory:

  • Create a new file, AudienceSegmentationErrorWidget.js, for the Full Width Error Banner component.
  • It can follow a similar structure as the AudienceSegmentationSetupCTAWidget component.
  • It should receive a list of errors as a prop.
  • Wrap the body with the Widget component and add the noPadding prop to it to layout the SVG graphics.
  • Within the Widget component, use Grid->Row->Cell to arrange the error notice and buttons.
  • Use the h3 element for the error notice.
  • Export the error SVG from the Figma design and render it in the component.
  • Check for "insufficient permissions" errors using the isInsufficientPermissionsError helper function and render the Insufficient permissions copy.
  • Otherwise, render the Your visitor groups data loading failed copy.
  • Render the ReportErrorActions component with the following props, which will render the error variant-specific buttons appropriately (See MetricTileWrapper) :
    • moduleSlug - pass the analytics-4 module slug.
    • error - pass the errors.
    • hideRetryHelpLink - a boolean prop to hide the Get help link if the error is not a permissions error.
    • GetHelpLink - Create a new GetHelpLink component that renders the Contact your administrator. Trouble getting access? copy and the Get help link. It should be similar to the assets/js/components/KeyMetrics/GetHelpLink.js component.
    • getHelpClassName - custom class name for the Get help link.
  • Adjust the styles that match the Figma design.

In assets/js/components/ReportErrorActions.js:

  • Add a new boolean prop hideRetryHelpLink.
  • Render the Retry get help link if hideRetryHelpLink is falsy. See:
    <span className="googlesitekit-error-retry-text">
    { createInterpolateElement(
    __(
    'Retry didn’t work? <HelpLink />',
    'google-site-kit'
    ),
    {
    HelpLink: (
    <Link
    href={ errorTroubleshootingLinkURL }
    external
    hideExternalIndicator
    >
    { __( 'Get help', 'google-site-kit' ) }
    </Link>
    ),
    }
    ) }
    </span>

Test Coverage

  • Add storybook stories for the AudienceSegmentationErrorWidget component.
  • Add test coverage for the AudienceSegmentationErrorWidget component, particularly around the retry logic and different conditions for rendering different sets of copy.

QA Brief

Changelog entry

  • Add the Full Width Error Banner for the Audience Segmentation feature 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 Feb 6, 2024
@ivonac4 ivonac4 added Next Up Issues to prioritize for definition and removed Next Up Issues to prioritize for definition labels Feb 9, 2024
@ivonac4 ivonac4 added the Next Up Issues to prioritize for definition label Mar 6, 2024
@techanvil techanvil assigned techanvil and unassigned techanvil Mar 18, 2024
@hussain-t hussain-t assigned hussain-t and unassigned hussain-t Mar 19, 2024
@tofumatt tofumatt self-assigned this Mar 20, 2024
@tofumatt
Copy link
Collaborator

IB ✅

@tofumatt tofumatt removed their assignment Mar 20, 2024
@tofumatt tofumatt removed the Next Up Issues to prioritize for definition label Mar 20, 2024
@hussain-t hussain-t self-assigned this Mar 21, 2024
@ivonac4 ivonac4 added the Squad 2 (Team M) Issues for Squad 2 label Apr 3, 2024
@hussain-t hussain-t removed their assignment Apr 24, 2024
@techanvil techanvil assigned techanvil and hussain-t and unassigned techanvil Apr 24, 2024
@hussain-t hussain-t assigned techanvil and unassigned hussain-t Apr 25, 2024
techanvil added a commit that referenced this issue Apr 25, 2024
@kelvinballoo
Copy link
Collaborator

QA Update ❌

Tested on Storybook and documented my issues below:

ITEM 1:
Font weight for 'Retry' button should be 500 instead of 400
Font should be 'Google sans Text' but it's 'Google Sans Display' for the Main title.
These are applicable for both the default and the permission error states.

Implementation: The font weight is currently 400: Screenshot 2024-04-28 at 23 32 20

Figma: The font weight should be 500.
Screenshot 2024-04-28 at 23 33 23

__________________________________________________

ITEM 2:

  • Distance between msg and button should be 14px instead of 18px
Figma: Screenshot 2024-04-28 at 23 35 38

Implementation: The gap is currently at 18px
Screenshot 2024-04-28 at 23 36 58

__________________________________________________

ITEM 3:

  • The top of the image should be 20px from the top of the block but it's more than that currently.
  • Similarly at the bottom, the gap should have been 20px below the image but it's less than that.
    Refer to the details for more details:
Figma: Screenshot 2024-04-28 at 23 39 46

Implementation:
Screenshot 2024-04-28 at 23 41 58


ITEM 4:
Overall button size on Figma is 68 x 32 px
And the Top and bottom margin inside button should be 6px.

If you look at the current implementation, the overall button size is (33.75+16+16) x (8+24+8) = 65.75 x 40px
And the Top and bottom margin inside button is 8px.

Figma: Screenshot 2024-04-28 at 23 46 27

Implementation:
Screenshot 2024-04-28 at 23 48 34

__________________________________________________

ITEM 5:
We don't have a design on mobile but I feel that we can set the top and bottom spaces more equal for a better visual?
Currently, the top section is bigger than the bottom.

Screenshot 2024-04-28 at 23 54 56

ITEM 6:
There is a very weird behaviour at breakpoint 960px.
At 959px, the image is at the left.
At 960px, it's at the centre.
At 961px, it's at the right.
While we are transitioning from tablet to desktop, I don't know why we have an odd one at 960px. It feels weird to me.
Let me know if there is a valid reason for this.

959px: Screenshot 2024-04-28 at 23 57 41

960px:
Screenshot 2024-04-28 at 23 57 35

961px:
Screenshot 2024-04-28 at 23 57 49

@hussain-t
Copy link
Collaborator

hussain-t commented Apr 29, 2024

Thanks for raising the issues, @kelvinballoo. I've addressed most of the observations in the follow-up PR. Please note that the Figma designs can be a bit out of alignment with the actual plugin implementation. You can see examples in the plugin. We should keep it consistent with the existing styles.

ITEM 1:
Font weight for 'Retry' button should be 500 instead of 400
Font should be 'Google sans Text' but it's 'Google Sans Display' for the Main title.
These are applicable for both the default and the permission error states.

Fixed ✅

ITEM 2:
Distance between msg and button should be 14px instead of 18px

Fixed ✅

ITEM 3:
The top of the image should be 20px from the top of the block but it's more than that currently.
Similarly at the bottom, the gap should have been 20px below the image but it's less than that.
Refer to the details for more details:

Fixed ✅

ITEM 4:
Overall button size on Figma is 68 x 32 px
And the Top and bottom margin inside button should be 6px.
If you look at the current implementation, the overall button size is (33.75+16+16) x (8+24+8) = 65.75 x 40px
And the Top and bottom margin inside button is 8px.

Figma design can be a bit out of alignment with the actual plugin implementation. We should keep it consistent with the existing styles. The current implementation of the button aligns with the existing styles.

ITEM 5:
We don't have a design on mobile but I feel that we can set the top and bottom spaces more equal for a better visual?
Currently, the top section is bigger than the bottom.

Fixed ✅

ITEM 6:
There is a very weird behaviour at breakpoint 960px.

Fixed ✅

@hussain-t
Copy link
Collaborator

@techanvil, back to you to review the follow-up PR. Thanks!

techanvil added a commit that referenced this issue Apr 29, 2024
Enhance - #8230 - Add the Full Width Error Banner (follow-up)
@techanvil techanvil assigned kelvinballoo and unassigned techanvil Apr 29, 2024
@techanvil
Copy link
Collaborator Author

Thanks @hussain-t!

Back to you for another pass, @kelvinballoo.

@kelvinballoo
Copy link
Collaborator

QA Update ⚠️

Thanks @hussain-t .
Tested good overall.

Need your help to check on item 5.

ITEM 1:
For both the default and the permission error states:

  • Font weight for buttons are now 500
  • Font is now 'Google sans Text' for the Main title.
________________________________________________________________________

ITEM 2:
Distance between msg and button is now 14px.

Screenshot 2024-04-30 at 15 06 44

ITEM 3:
The top and bottom of the image is now 20px.

Screenshot 2024-04-30 at 15 19 18

ITEM 4:
Noted that we are keeping things consistent with other areas of the plugin. Figma is not consistent for this.


ITEM 5: ⚠️
While the bottom and top margin are the same, it does still feel like the previous one.
@hussain-t , could you elaborate on the changes done for this one?

Screenshot 2024-04-30 at 15 33 08

ITEM 6:
The 960px breakpoint follows that of 959px and that's more consistent.
This is good.

Screenshot 2024-04-30 at 15 35 06

@hussain-t
Copy link
Collaborator

Hi @kelvinballoo,

ITEM 5: ⚠️
While the bottom and top margin are the same, it does still feel like the previous one.

I have addressed this issue in a new follow-up PR. I've also fixed the spacing for the tablet viewport.

@techanvil, back to you for another review.

@hussain-t hussain-t assigned techanvil and unassigned hussain-t Apr 30, 2024
techanvil added a commit that referenced this issue Apr 30, 2024
…w-up

Enhance/#8230 - Add the Full Width Error Banner (follow-up)
@techanvil
Copy link
Collaborator Author

Thanks @hussain-t, that's merged 👍

Back to you @kelvinballoo!

@techanvil techanvil assigned kelvinballoo and unassigned techanvil Apr 30, 2024
@kelvinballoo
Copy link
Collaborator

QA Update ✅

This is looking good on mobile and tablet viewports now.

Screenshot 2024-04-30 at 23 14 57
Screenshot 2024-04-30 at 23 13 31

Moving ticket to Approval.

@kelvinballoo kelvinballoo removed their assignment Apr 30, 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 Squad 2 (Team M) Issues for Squad 2 Type: Enhancement Improvement of an existing feature
Projects
None yet
Development

No branches or pull requests

6 participants