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

Fix heading styles in Glossary Definition and Tooltip #13205

Merged
merged 4 commits into from
Jun 27, 2024

Conversation

pettinarip
Copy link
Member

In recent changes, we have changed a bit how we display the heading on the Glossary Definition component, and this affects the Glossary Tooltip styles.

Description

This PR fixes the style issues by:

  • using the new Heading component with DS styles
  • removing the GlossaryDefinition dependency on GlossaryTooltip since they look quite different now (no more reusability here)

Bonus: added a new story for the open state of GlossaryTooltip to avoid getting these regressions again.

Copy link

netlify bot commented Jun 19, 2024

Deploy Preview for ethereumorg ready!

Name Link
🔨 Latest commit aa21459
🔍 Latest deploy log https://app.netlify.com/sites/ethereumorg/deploys/6679c034dc877f0008e7f5f6
😎 Deploy Preview https://deploy-preview-13205--ethereumorg.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.
Lighthouse
Lighthouse
7 paths audited
Performance: 51 (🟢 up 4 from production)
Accessibility: 92 (no change from production)
Best Practices: 84 (🔴 down 8 from production)
SEO: 93 (no change from production)
PWA: -
View the detailed breakdown and full score reports

To edit notification comments on pull requests, go to your Netlify site configuration.

Copy link
Contributor

@TylerAPfledderer TylerAPfledderer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@pettinarip quick thought from me 😁

export const Basic: Story = {}

// for chromatic story snapshot showing the rendered popover
export const OnOpen: Story = {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Need to add a decorator here, where it provides enough white-space to cover the visible tooltip. Otherwise the tooltip will be cropped out in the chromatic snapshot.

The decorator can be supplied at the individual story level. (There is currently a decorator at the component level for the Tooltip for this very reason)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah! great point. Thanks for sharing, I was thinking about how to deal with this issue and couldn't remember what you did.

@corwintines corwintines self-assigned this Jun 24, 2024
@github-actions github-actions bot added the tooling 🔧 Changes related to tooling of the project label Jun 24, 2024
@corwintines
Copy link
Member

@pettinarip @TylerAPfledderer I think this will be ready now

I added the glossary-tooltip namespace into .storybook/i18next.ts, and changed the term from big-endian to bridge since big-endian isn't in glossary-tooltip.json.

@wackerow
Copy link
Member

@nloureiro Mind approving these changes in Chromatic if they look okay to you?

https://www.chromatic.com/review?appId=63b7ea99632763723c7f4d6b&number=13205&type=linked&view=changes

@pettinarip
Copy link
Member Author

@pettinarip @TylerAPfledderer I think this will be ready now

I added the glossary-tooltip namespace into .storybook/i18next.ts, and changed the term from big-endian to bridge since big-endian isn't in glossary-tooltip.json.

Nice! thanks!

@nloureiro
Copy link
Contributor

@nloureiro Mind approving these changes in Chromatic if they look okay to you?

https://www.chromatic.com/review?appId=63b7ea99632763723c7f4d6b&number=13205&type=linked&view=changes

looks good to me. approved on Chromatic :)

@pettinarip pettinarip merged commit 26bb423 into dev Jun 27, 2024
10 checks passed
@pettinarip pettinarip deleted the fix-glossary-tooltip-styles branch June 27, 2024 11:45
This was referenced Jul 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
tooling 🔧 Changes related to tooling of the project
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants