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

Tooltip background is different between Figma and test site #5444

Closed
eclarke1 opened this issue Jun 27, 2022 · 5 comments
Closed

Tooltip background is different between Figma and test site #5444

eclarke1 opened this issue Jun 27, 2022 · 5 comments
Labels
P1 Medium priority Type: Enhancement Improvement of an existing feature

Comments

@eclarke1
Copy link
Collaborator

eclarke1 commented Jun 27, 2022

Feature Description

Bug bash issue: https://app.asana.com/0/1202258919887896/1202422111202705 please see Asana for background

On the figma designs the tool tip (when you have 2 or more admins) is white background with blank text. Where as on the test site it is grey background with black text. This is quite hard to read. Also the font size is bigger on the figma designs, and easier to read.

Figma:
image.png

Chrome:
image.png


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

Acceptance criteria

  • A new Tooltip component should be created to replace our existing usage of the Material UI <Tooltip> component.
  • The new component should be based on Material UI's Tooltip, but visually match the style of tooltip used in the [Dashboard Sharing Figma designs].
  • All current instances of import { Tooltip } from '@material-ui/core'; should be replaced with our own Tooltip component.

Implementation Brief

PoC PR

A proof-of-concept PR has been added with most of the implementation mentioned below here. This can be used as a starting point, ensuring these next steps:

  • There are no console errors.
  • Double-check all the file headers, DocBlocks, and comments.
  • The styling is accurate.

A summary of the implementation is still mentioned below:

  • Rename the existing Tooltip.js file in the assets/js/components folder to something like JoyrideTooltip.js or FeatureTourTooltip.js, since it is only used in a few places and mostly within feature tours. Also, update its component definition, stories, and usages across the codebase.
  • In the same folder, create a new file named Tooltip.js, which will contain the generic tooltip component that we will create in this issue. In this file:
    • We will basically create a wrapper for the Tooltip component from @material-ui/core with our own CSS selectors and styling.
    • Export a new component named Tooltip which should explicitly accept the prop children. The remaining props can be assigned to props (e.g. ...props).
    • Import the Tooltip component with an alias (e.g. MuiTooltip) from @material-ui/core.
    • Return MuiTooltip from the component. It should have:
      • A classes prop with an object containing a keys:
        • popper with the value of googlesitekit-tooltip-popper.
        • tooltip with the value of googlesitekit-tooltip.
        • An 'arrow' prop should be passed on (with no value, treating it as true).
      • The remaining props assigned to props passed to this component as well (e.g. { ...props }).
      • The children prop should be passed as the child for this component.
  • In assets/sass/components/global/_googlesitekit-tooltip.scss, add the necessary styles according to Figma to the .googlesitekit-tooltip.MuiTooltip-tooltip and the .googlesitekit-tooltip .MuiTooltip-arrow selectors.
  • Replace the usages of @material-ui/core Tooltip component with the new Tooltip component.
  • In the existing usages, remove the classes prop as it is already added in our new component.
  • Add a Storybook story for this component as well.

Test Coverage

  • No tests need to be added/updated.

QA Brief

  • Go to the Site Kit Dashboard.
  • Verify that the tooltips within the dashboard appear according to the designs included in the issue description. Some tooltips to check include:
    • Header action items (e.g. Search, Date Range button, Dashboard Sharing Settings button, Help button, User Profile button).
    • Dashboard sharing settings modal info tooltips (shown in the task description).

Changelog entry

  • Update tooltip styles.
@eclarke1 eclarke1 added P1 Medium priority Type: Enhancement Improvement of an existing feature labels Jun 27, 2022
@tofumatt tofumatt self-assigned this Jun 28, 2022
@tofumatt
Copy link
Collaborator

This isn't a Dashboard-sharing specific issue: the Tooltip in question is imported from Material UI and used both in Dashboard sharing UI and outside it.

That said: the proposed improvements to the tooltip style in the Dashboard Sharing Figma designs are pleasant and more readable.

I've added ACs but removed this from the Dashboard Sharing epic.

@tofumatt tofumatt removed their assignment Jun 28, 2022
@nfmohit nfmohit self-assigned this Aug 8, 2022
@nfmohit nfmohit mentioned this issue Aug 9, 2022
18 tasks
@nfmohit nfmohit removed their assignment Aug 10, 2022
@eugene-manuilov eugene-manuilov self-assigned this Aug 10, 2022
@eugene-manuilov
Copy link
Collaborator

A proof-of-concept PR has been added with most of the implementation mentioned below here. This can be used as a starting point, ensuring these next steps:

@nfmohit, please, avoid using the PR as IB approach, this is strongly discouraged because time spent on actual execution for your PoC hasn't been allocated for this ticket yet. Try to describe steps that are needed without coding, if something is not clear or needs discovery, then just mention it in IB.

  • We will basically create a wrapper for the Tooltip component from @material-ui/core with our own CSS selectors and styling.

We also need to display an arrow that will point to the target element. There is an example in the documentation that has the arrow, we need to do the same. Could you please add it to IB?

@nfmohit
Copy link
Collaborator

nfmohit commented Aug 10, 2022

@eugene-manuilov Thank you very much for the kind and detailed IBR!

please, avoid using the PR as IB approach, this is strongly discouraged because time spent on actual execution for your PoC hasn't been allocated for this ticket yet. Try to describe steps that are needed without coding, if something is not clear or needs discovery, then just mention it in IB.

Noted, I'll keep that in mind and not add it in future unless absolutely necessary. Thanks!

We also need to display an arrow that will point to the target element. There is an example in the documentation that has the arrow, we need to do the same. Could you please add it to IB?

Added, thank you!

@nfmohit nfmohit assigned eugene-manuilov and unassigned nfmohit Aug 10, 2022
@eugene-manuilov
Copy link
Collaborator

Thanks, @nfmohit. IB ✔️

@eugene-manuilov eugene-manuilov removed their assignment Aug 10, 2022
@nfmohit nfmohit self-assigned this Aug 15, 2022
@nfmohit nfmohit removed their assignment Aug 15, 2022
@eugene-manuilov eugene-manuilov self-assigned this Aug 15, 2022
eugene-manuilov added a commit that referenced this issue Aug 15, 2022
@eugene-manuilov eugene-manuilov removed their assignment Aug 15, 2022
@mohitwp mohitwp self-assigned this Aug 16, 2022
@mohitwp
Copy link
Collaborator

mohitwp commented Aug 17, 2022

QA Update ✅

  • Verified tooltips within the dashboard appear according to the designs in issue description.
  • Verified Header action items (e.g. Search, Date Range button, Dashboard Sharing Settings button, Help button, User Profile button).
  • Verified Dashboard sharing settings modal info tooltips.
  • Verified both main and entity dashboard.

image

image

image

image

image

image

image

image

image

image

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
P1 Medium priority Type: Enhancement Improvement of an existing feature
Projects
None yet
Development

No branches or pull requests

6 participants