[UI lib] Button, Link & ButtonLink components #7438

pettinarip · 2022-08-15T12:17:52Z

Migration of Button, Link & ButtonLink components.

Description

This PR includes the migration of some core components that are a bit related between them. The PR does not only migrate the components to chakra, it also includes refactors on each one of them.

Notes for the reviewer:

The main files to review are:
- in the theme configuration folder (src/@chakra-ui)
- and the mentioned components on the title
The rest are little adjustments to satisfy the newer components.
As this PR touches all the pages, lets try to test as many pages as we can.
The Button styles are the new styles from the design system.

Things to test or consider:

Button & Link styles
- in light & dark mode
- states (hover, active, disabled)
Link functionality
- Intl support
- RTL support
- External, hashed, static links

Test env: https://ethereumorgwebsitedev01-chakrabuttonlinkcomponent.gtsb.io/

Figma: https://www.figma.com/file/NrNxGjBL0Yl1PrNrOT8G2B/ethereum.org-Design-System

Related Issue

Epic #6374

gatsby-cloud · 2022-08-15T14:45:59Z

Gatsby Cloud Build Report

ethereum-org-website-dev

🎉 Your build was successful! See the Deploy preview here.

Build Details

View the build logs here.

🕐 Build time: 20m

pettinarip · 2022-08-15T15:56:22Z

src/components/Button.tsx

+  toId?: string
+}
+
+const Button: React.FC<IProps> = ({ toId, children, ...props }) => {


I'm calling this component Button bc it is the same Chakra button but with the scrolling feature. This way, you will just use this button component in the codebase.

import Button from 'components/Button'

On the other hand, we could break them out by calling this component ButtonScroll and using it only where we need to use the auto-scrolling.

import ButtonScroll from 'components/ButtonScroll' // for the rest, we use the native chakra button import Button from 'chakra/Button'

I just wanted to check if you have a preference.

pettinarip · 2022-08-15T17:58:51Z

src/components/FeedbackWidget.tsx

+                  <Button
+                    variant="outline"
+                    color="primary"
+                    borderColor="primary"


TODO: create new variant for this case (is defined in the design system)

pettinarip · 2022-08-15T18:01:49Z

src/components/Link.tsx

-    throw new Error("Either 'to' or 'href' props must be provided")
-  }
+  // TODO: in the next PR we are going to deprecate `to` prop and just use `href`
+  const to = (toProp || href)!


In a next PR we are going to just use href for everything. I didn't want to include that change here since it touches a massive amount of files.

pettinarip · 2022-08-16T01:41:22Z

gatsby-config.ts

@@ -194,7 +194,7 @@ const config: GatsbyConfig = {
    {
      resolve: "@chakra-ui/gatsby-plugin",
      options: {
-        resetCSS: false,
+        resetCSS: true,


We have enabled the reset from Chakra. It does seem to be working fine so far. I haven't seen any conflict yet.

nloureiro · 2022-08-30T08:50:51Z

The focus on the links is too strong. We don't have this on production. We probably need to rethink the focus style on the link.

Screen.Recording.2022-08-30.at.09.41.21.mov

I got it; it's the box shadow that has to disappear.

Screen.Recording.2022-08-30.at.09.48.55.mov

pettinarip · 2022-08-30T12:59:24Z

@nloureiro thanks for the review.

re: focus border, I agree that the border is too thick & doesn't look nice on the links. I don't like too much the idea of hiding it until we rethink it. I'll try to copy the previous version to have at least some focus indicator for a11y purposes.

What do you think?

re: the other issues:

The initial flashing will be covered in a different PR since it is an already known issue.
The other problems where you have a mixed color mode in different sections, thats weird. I think is the same issue @minimalsm is having with the breadcrumbs colors. I couldn't reproduce it in my local or in the test env....I think it is related to some weird state in the localstorage. Would you be able to clear the localstorage and test it again?

nloureiro · 2022-08-30T13:38:22Z

@nloureiro thanks for the review.

re: focus border, I agree that the border is too thick & doesn't look nice on the links. I don't like too much the idea of hiding it until we rethink it. I'll try to copy the previous version to have at least some focus indicator for a11y purposes. What do you think?

re: the other issues:

The initial flashing will be covered in a different PR since it is an already known issue.

The other problems where you have a mixed color mode in different sections, thats weird. I think is the same issue @minimalsm is having with the breadcrumbs colors. I couldn't reproduce it in my local or in the test env....I think it is related to some weird state in the localstorage. Would you be able to clear the localstorage and test it again?

regarding the :focus
I'm not sure of your idea here, but that will affect all the tags, right?
How would it work in the side navigation?

we should have :focus for accessibility affordance, what do you think about this for now? then I will revisit this on the Design system and open a PR to fix accordantly.
@minimalsm and @konopkja : any thoughts here?

minimalsm · 2022-08-30T13:47:12Z

@pettinarip yeah, I can't reproduce anymore (even without clearing my local storage). I'm not sure what's going on.

====

Re: focus. I don't think the focus style is too much. The weird part is that it is persisting on page transitions. Can we stop that?

I also (personally) wouldn't have onClick focus as the most common convention seems to be for keyboard usage only (but I could be wrong here)

Edit: This appears to be an issue with Chakra. The problem is resolved in their Storybook.. Bumping Chakra version might solve this? cc: @nloureiro @pettinarip

pettinarip · 2022-08-30T14:03:22Z

@nloureiro I like that style. I'll implement that for now.

@minimalsm interesting. Yes, I can confirm it is a v2 improvement. But good for us, we have already a PR opened for that #7617 👍🏼

nloureiro · 2022-08-30T14:27:11Z

@minimalsm : to reproduce the theme bug

follow this link in safari > https://ethereumorgwebsitedev01-chakrabuttonlinkcomponent.gtsb.io/en/
change the theme

corwintines · 2022-08-30T14:42:42Z

Was doing some testing on my end to see if I could replicate any of the themeing issues, but haven't been able to reproduce them on my side. Tested in Safari, Chrome, Brave, and Firefox.

pettinarip · 2022-08-30T21:23:42Z

@nloureiro @minimalsm @corwintines

I’ve updated the PR with the new focus style on the Links in order to let you guys see the results.

The problem is the inconsistency with other focus styles we have.

We are using links not just for text but sometimes to wrap icons or buttons

You can see on the nav dropdowns that the focus style is different. This is because those navs are not links, so they are not inheriting the link styles

Some cards are wrapped with a Link.

Another example of icons

I would propose to keep the previous focus styles (the ones we currently have in prod) for the links. To keep it simple for the scope of this PR & for consistency.

If you agree then I can modify them to be the same as before. Then, once we have a clear style for the focus in links, we can dedicate a new PR just for that.

In summary, my idea is:

Button: we apply the new focus styles (the ones that are in the Design System)
Link: keep the current styles that are in prod

nloureiro · 2022-08-31T08:21:25Z

@nloureiro @minimalsm @corwintines

I’ve updated the PR with the new focus style on the Links in order to let you guys see the results.

The problem is the inconsistency with other focus styles we have.

We are using links not just for text but sometimes to wrap icons or buttons

You can see on the nav dropdowns that the focus style is different. This is because those navs are not links, so they are not inheriting the link styles

Some cards are wrapped with a Link.

Another example of icons

I would propose to keep the previous focus styles (the ones we currently have in prod) for the links. To keep it simple for the scope of this PR & for consistency.

If you agree then I can modify them to be the same as before. Then, once we have a clear style for the focus in links, we can dedicate a new PR just for that.

In summary, my idea is:

Button: we apply the new focus styles (the ones that are in the Design System)

Link: keep the current styles that are in prod

errr... this is messy.
I've tried some variations and it's not easy to keep a consistent :focus beahiver with the actual code.
Probably we need to work this on a component level.

For these reason I agree on keep the production :focus, that is basically no :focus at all. Then we need to discuss how to precess here.

pettinarip · 2022-08-31T13:51:41Z

src/components/GlobalStyle.tsx

@@ -755,14 +755,15 @@ const GlobalStyle: React.FC = () => {
        /* Anchor tag styles */
        /* Selected specifically for mdx rendered side icon link */
        .header-anchor {
-          position: relative;
+          position: relative !important;


Due to some sort of build order, these styles are being overridden by the default styles that come with the plugin https://github.com/gatsbyjs/gatsby/blob/master/packages/gatsby-remark-autolink-headers/src/gatsby-ssr.js#L15 thats why I had to put those !important.

I'll see if I can revert these changes in the #7564 PR.

pettinarip · 2022-08-31T14:08:21Z

Cool! I've updated the PR leaving the :focus-visible as outline: auto, to be handled by the browser.

minimalsm

This is what I'm getting now for focus state.

This does not seem like very good for accessibility.

Given that the Chakra update removes the on-click focus and the page transition issue (which I found out is actually strict WCAG, but we don't need to follow IMO), would we be better just keeping the focus styling before the revisions here?

corwintines

Nice job @pettinarip!

samajammin · 2022-09-15T04:36:52Z

src/components/FeedbackCard.tsx

-  svg {
-    height: 1.5rem;
-    &.flip {
-      transform: scaleY(-1);


I believe we still need this transform: https://discord.com/channels/714888181740339261/938069231570923550/1019727618666410034

pettinarip added 10 commits August 10, 2022 15:06

migrate Link to use chakra

d72276d

fix footer link

e82c63d

remove active styles as we are not using them

d40a004

fix typing issues

80ef4d0

fix styles on wrapped Links

ed06eb8

initial refactor of Buttons and ButtonLinks

e3f03e2

Merge branch 'dev' into chakra-buttonlink-component

89dd33f

fix button styles

032a1bc

fix types

321e53d

revert dir prop

bb06888

github-actions bot added content 🖋️ This involves copy additions or edits review needed 👀 labels Aug 15, 2022

pettinarip mentioned this pull request Aug 15, 2022

Migrate Link component to Chakra UI #7275

Closed

pettinarip commented Aug 15, 2022

View reviewed changes

pettinarip added 3 commits August 15, 2022 13:22

remove old comments

72b33f5

remove constants file since it was causing an error in the build

416ee6a

enable chakra resetCSS flag

08bf701

pettinarip commented Aug 15, 2022

View reviewed changes

pettinarip added 3 commits August 15, 2022 15:59

implement button outline-color variant

4a473d2

adopt design system colors

c23aa97

Merge branch 'dev' into chakra-buttonlink-component

575f4f2

pettinarip commented Aug 16, 2022

View reviewed changes

pettinarip self-assigned this Aug 16, 2022

pettinarip marked this pull request as ready for review August 16, 2022 01:41

pettinarip requested review from samajammin, wackerow, corwintines and minimalsm as code owners August 16, 2022 01:41

change focus styles on Links

23d6bf1

nloureiro mentioned this pull request Aug 30, 2022

React 18 #7617

Merged

fix action card outline on focus

8102b6e

vercel bot had a problem deploying to Preview August 30, 2022 21:10 Failure

pettinarip added 2 commits August 30, 2022 18:45

Merge branch 'dev' into chakra-buttonlink-component

32b69e4

fix old style prop name

6008fbe

pettinarip added 2 commits August 31, 2022 10:20

set link outline styles as auto

5a40de1

tmp fix on header anchors styles

3372f93

pettinarip commented Aug 31, 2022

View reviewed changes

vercel bot had a problem deploying to Preview August 31, 2022 14:08 Failure

revert hidden outline in ActionCard

d1855b4

minimalsm reviewed Aug 31, 2022

View reviewed changes

vercel bot had a problem deploying to Preview August 31, 2022 16:30 Failure

corwintines approved these changes Aug 31, 2022

View reviewed changes

minimalsm approved these changes Sep 1, 2022

View reviewed changes

pettinarip merged commit 593a84c into dev Sep 1, 2022

pettinarip deleted the chakra-buttonlink-component branch September 1, 2022 15:44

corwintines mentioned this pull request Sep 1, 2022

Deploy v6.0.0 #7658

Merged

samajammin reviewed Sep 15, 2022

View reviewed changes

pettinarip mentioned this pull request Sep 24, 2022

Epic: Implement UI library #6374

Closed

80 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[UI lib] Button, Link & ButtonLink components #7438

[UI lib] Button, Link & ButtonLink components #7438

pettinarip commented Aug 15, 2022 •

edited

Loading

gatsby-cloud bot commented Aug 15, 2022 •

edited

Loading

pettinarip Aug 15, 2022

pettinarip Aug 15, 2022 •

edited

Loading

pettinarip Aug 15, 2022 •

edited

Loading

samajammin Aug 18, 2022

pettinarip Aug 16, 2022

nloureiro commented Aug 30, 2022

pettinarip commented Aug 30, 2022

nloureiro commented Aug 30, 2022

minimalsm commented Aug 30, 2022 •

edited

Loading

pettinarip commented Aug 30, 2022

nloureiro commented Aug 30, 2022

corwintines commented Aug 30, 2022

pettinarip commented Aug 30, 2022

nloureiro commented Aug 31, 2022

pettinarip Aug 31, 2022

pettinarip commented Aug 31, 2022

minimalsm left a comment

corwintines left a comment

samajammin Sep 15, 2022

[UI lib] Button, Link & ButtonLink components #7438

[UI lib] Button, Link & ButtonLink components #7438

Conversation

pettinarip commented Aug 15, 2022 • edited Loading

Description

Related Issue

gatsby-cloud bot commented Aug 15, 2022 • edited Loading

Gatsby Cloud Build Report

Build Details

pettinarip Aug 15, 2022

Choose a reason for hiding this comment

pettinarip Aug 15, 2022 • edited Loading

Choose a reason for hiding this comment

pettinarip Aug 15, 2022 • edited Loading

Choose a reason for hiding this comment

samajammin Aug 18, 2022

Choose a reason for hiding this comment

pettinarip Aug 16, 2022

Choose a reason for hiding this comment

nloureiro commented Aug 30, 2022

pettinarip commented Aug 30, 2022

nloureiro commented Aug 30, 2022

minimalsm commented Aug 30, 2022 • edited Loading

pettinarip commented Aug 30, 2022

nloureiro commented Aug 30, 2022

corwintines commented Aug 30, 2022

pettinarip commented Aug 30, 2022

nloureiro commented Aug 31, 2022

pettinarip Aug 31, 2022

Choose a reason for hiding this comment

pettinarip commented Aug 31, 2022

minimalsm left a comment

Choose a reason for hiding this comment

corwintines left a comment

Choose a reason for hiding this comment

samajammin Sep 15, 2022

Choose a reason for hiding this comment

pettinarip commented Aug 15, 2022 •

edited

Loading

gatsby-cloud bot commented Aug 15, 2022 •

edited

Loading

pettinarip Aug 15, 2022 •

edited

Loading

pettinarip Aug 15, 2022 •

edited

Loading

minimalsm commented Aug 30, 2022 •

edited

Loading