feat: Update Text and Heading Themes for New DS #10606

TylerAPfledderer · 2023-07-04T21:13:10Z

Description

Update theming of the Heading and Text components with stories.

Related Issue

Part of the Implentation of the DS v1 #9548

gatsby-cloud · 2023-07-04T21:53:43Z

✅ ethereum-org-website-dev deploy preview ready

Deploy preview
Build logs · 25m build time

…stories

TylerAPfledderer · 2023-07-07T01:20:54Z

@nloureiro Please view the Chromatic UI Tests check and storybook. Of course, this is considered a priority to be able to properly work on other components that need it.

One thing outstanding on here: should a margin bottom be a part of the Heading styling -- to be specified for each size in the theme -- or be kept separate and only be added as the Heading is used?

…e-new-ds

pettinarip · 2023-07-27T20:31:49Z

@nloureiro Please view the Chromatic UI Tests check and storybook. Of course, this is considered a priority to be able to properly work on other components that need it.

One thing outstanding on here: should a margin bottom be a part of the Heading styling -- to be specified for each size in the theme -- or be kept separate and only be added as the Heading is used?

Interesting point @TylerAPfledderer. IMO for this first iteration we should not add those margins to the Heading component.

Later, as we migrate the different layouts or pages (previously defined in the DS), we are going to start detecting patterns with these margins, and we should decide then if we add them to the Headings or if we do something else, like using textStyles (I'd go with adding them to the Headings and then overriding the other cases were we are not using the default margins).

src/@chakra-ui/gatsby-plugin/components/Heading.ts

pettinarip · 2023-07-28T12:09:03Z

src/@chakra-ui/gatsby-plugin/styles.ts

    },
    a: {
      color: "primary.base",
      textDecoration: "underline",
    },
-    // should be replace with https://chakra-ui.com/docs/components/text
-    p: {
-      margin: "0px 0px 1.45rem",


Now, looking at the preview deploy, I see that we kind of depend on this margin to avoid having to refactor all the codebase. I'd say that for the Text component we add this button margin. What do you think @nloureiro and @TylerAPfledderer?

Note for context for @nloureiro. This Text component represents the p tag, the paragraphs in html.

@pettinarip fair point!

So for example, with the Homepage hero where this margin would be used against the CTA button "Explore Ethereum", when working on the new heroes I took a different approach, whereby I structured the Header and text under one flex stack, and then that grouping with the button at a higher level stack, and used the gap or spacing prop to define the "margin" between the different texts and between the paragraph and the button.

// Something to this effect <VStack spacing='4'> <VStack spacing='2'> <Heading /> <Text /> </VStack> <Button /> </VStack>

I should ask then what would best practice be here? In my description above, this approach using the Flexbox gap property to define the margins between content instead of using the margin prop.

And I would consider in addition to the above the idea of using margin only when we are stacking paragraphs, with a defined :first- or :last-of-type selector so that only margin is defined between the paragraphs, but not added above the first or below the last of the stacked siblings.

So:

"p:not(:last-of-type)": { mb: "2" // or some other token value }

However, I also understand that margin has to be well defined for the mdx file parsing because you have no ability to isolate groups of text elements into neatly packaged boxes. So I would want to figure out what the best practice should be to keep usage in MDX pages inline with the page layouts that do not use MDX.

Using gap and spacing is correct and expected as the best way to go looking ahead.

Now, talking about the current problem and thinking about how to support the old margins, I think that the best way to go is by leaving the Text component as it is and creating a new temporary one called OldText that would have the old margins.

Following this idea

the Text component would be used in all new implementation

the OldText would be marked as deprecated and would be used in the rest of the old/current implementations.

The OldText component might be something as simple as this

function OldText(props) { return <Text margin={oldDefaultMargins} {...props} /> }

With that new component in place, I could go and refactor the entire codebase by importing the OldText and adding those changes to this PR.

Then, as you migrate the new components as the Hero, you can start using and importing the native Text.

Does it make sense?

@pettinarip Ok! So using the OldText component for the MDX files too, correct?

@pettinarip I've pushed this new component here.

Thanks. I'll refactor the code to use that component instead of the Text.

Sounds good!

…e-new-ds

pettinarip · 2023-08-07T16:11:55Z

src/@chakra-ui/gatsby-plugin/components/Text.ts

@@ -0,0 +1,48 @@
+import { defineStyle, defineStyleConfig } from "@chakra-ui/react"
+
+const sizes = {


@TylerAPfledderer we discussed with Nuno about these sizes and we've updated them to match the same sizes than Chakra https://chakra-ui.com/docs/styled-system/theme#typography

Now, md should be the same md as in Chakra (1 rem). Same with the rest.

@pettinarip to clarify, this is just regarding the Text component? It looks like the sizes for the Heading component have not changed (which is to be expected since they are currently inline with the Chakra defaults)

Also need clarification regarding the body copy with this update. Marked a question in the figma file as I found a discrepancy.

Correct @TylerAPfledderer. The Heading has not suffered any update.

@pettinarip this is now updated in the PR

nloureiro · 2023-08-14T14:49:06Z

Maybe late to this one. Where should I look to review?

here?

TylerAPfledderer · 2023-08-17T18:53:43Z

Maybe late to this one. Where should I look to review?

here?

@pettinarip looks like Chromatic wasn't triggered with the new stories.

@nloureiro yes to this one as well as Atoms / Typography / Text

…e-new-ds

TylerAPfledderer · 2023-08-21T02:34:32Z

@pettinarip @nloureiro I updated the branch recently, and now the chromatic test and review checks are now available!

NOTE

I've failed the UI Tests check due to regressions showing from the Card and Slider stories. (Glad they were available!)

Therefore, this PR should not merge until those at least can be fixed and shown as properly laid out (i.e. keep to the original spacing and sizing... or close to it). Then approve the UI Tests again and the UI review itself.

And this is why we have Chromatic! 🤣

Side Note: if both of you think that it would be simpler to only need the UI Tests check instead of also having the UI Reviews check, you should be able to disable it in the "Manage Page" section of the Chromatic Dashboard.
This means you would only prefer just approving/deny UI changes, and not doing overview approval like you would you request for any other stake-holders.

TylerAPfledderer · 2023-08-22T03:40:47Z

@nloureiro @pettinarip this should be good for a review again!

nloureiro · 2023-09-04T13:41:42Z

src/components/BaseStories/Heading.stories.tsx

+
+const headingScale: Array<HeadingProps> = [
+  {
+    as: "h1",


@TylerAPfledderer @pettinarip maybe this is normal, but having 3 times as="h1" is correct?

@nloureiro I could only assume that the three largest sizes would be reserved for the h1 heading. If that is true, then I would make sure that the story reflected that preference.

pettinarip

🚢

feat(theme/Heading): add new theme for DS

9511d07

github-actions bot added the review needed 👀 label Jul 4, 2023

TylerAPfledderer changed the title ~~feat(theme/Heading): add new theme for DS~~ feat: Update Text and Heading themes for New DS Jul 4, 2023

TylerAPfledderer changed the title ~~feat: Update Text and Heading themes for New DS~~ feat: Update Text and Heading Themes for New DS Jul 4, 2023

TylerAPfledderer added 4 commits July 5, 2023 10:01

feat(theme/Text): update theme for new DS

1585ecc

refactor(theme/Link): add sizes from Text theme and add Link to Text …

22b8d0c

…stories

refactor(theme/text): set global font size and add body copy story

de25c36

refactor(theme/styles): remove global styles for body copy and headings

97ce437

TylerAPfledderer force-pushed the feat/heading-text-theme-new-ds branch from 051c112 to 97ce437 Compare July 6, 2023 18:39

TylerAPfledderer marked this pull request as ready for review July 7, 2023 01:10

TylerAPfledderer requested review from samajammin, wackerow, corwintines, pettinarip, minimalsm, jmcook1186 and nhsz as code owners July 7, 2023 01:10

refactor(theme/Button): update Button component font size

adf11e6

TylerAPfledderer force-pushed the feat/heading-text-theme-new-ds branch from bf889e7 to adf11e6 Compare July 7, 2023 03:31

TylerAPfledderer added 3 commits July 7, 2023 00:18

Merge remote-tracking branch 'origin/dev' into feat/heading-text-them…

3ccf261

…e-new-ds

Merge remote-tracking branch 'origin/dev' into feat/heading-text-them…

22e3cd4

…e-new-ds

Merge remote-tracking branch 'origin/dev' into feat/heading-text-them…

db48b20

…e-new-ds

pettinarip mentioned this pull request Jul 19, 2023

Implementation of the DS v1 #9548

Open

pettinarip assigned TylerAPfledderer Jul 19, 2023

Merge remote-tracking branch 'origin/dev' into feat/heading-text-them…

136fd28

…e-new-ds

pettinarip reviewed Jul 27, 2023

View reviewed changes

src/@chakra-ui/gatsby-plugin/components/Heading.ts Show resolved Hide resolved

pettinarip reviewed Jul 28, 2023

View reviewed changes

TylerAPfledderer added 2 commits August 1, 2023 23:52

Merge remote-tracking branch 'origin/dev' into feat/heading-text-them…

e0173dc

…e-new-ds

feat: add OldText component

36e303b

pettinarip reviewed Aug 7, 2023

View reviewed changes

TylerAPfledderer added 2 commits August 8, 2023 09:10

refactor(theme/Text): update size token names to match updated DS

7b7e05c

refactor(theme/styles): shrink default body copy for desktop

b1f02f0

Merge remote-tracking branch 'origin/dev' into feat/heading-text-them…

c3a38a5

…e-new-ds

TylerAPfledderer added 3 commits August 21, 2023 23:12

fix(Card): update text size and spacing

7d7eeba

fix(Slider): update text sizing and spacing

4b35120

fix(Link.stories): add back spacing for link list

74e5dc8

pettinarip added 2 commits August 23, 2023 16:42

refactor code to use the new OldText component

cf974b8

Merge branch 'dev' into feat/heading-text-theme-new-ds

a500d33

github-actions bot added the content 🖋️ This involves copy additions or edits label Aug 30, 2023

pettinarip added 5 commits August 30, 2023 19:43

create new OldHeading to hold old styles and refactor codebase to use it

7f26b77

fixes

20d98a2

use chakra components

f2d3572

more fixes

6e56568

migrate all html headings to use Chakra Heading component

7fbab53

nloureiro approved these changes Sep 4, 2023

View reviewed changes

removed a extra space

af14c1b

pettinarip approved these changes Sep 11, 2023

View reviewed changes

pettinarip merged commit 4e89821 into ethereum:dev Sep 11, 2023

TylerAPfledderer deleted the feat/heading-text-theme-new-ds branch September 11, 2023 18:13

This was referenced Sep 13, 2023

Release candidate: v7.21.0 #11139

Merged

Deploy: v7.21.0 #11143

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Update Text and Heading Themes for New DS #10606

feat: Update Text and Heading Themes for New DS #10606

TylerAPfledderer commented Jul 4, 2023

gatsby-cloud bot commented Jul 4, 2023 •

edited

Loading

TylerAPfledderer commented Jul 7, 2023 •

edited

Loading

pettinarip commented Jul 27, 2023

pettinarip Jul 28, 2023

TylerAPfledderer Jul 31, 2023 •

edited

Loading

pettinarip Aug 1, 2023

TylerAPfledderer Aug 2, 2023

pettinarip Aug 2, 2023

TylerAPfledderer Aug 3, 2023

pettinarip Aug 4, 2023

TylerAPfledderer Aug 4, 2023

pettinarip Aug 7, 2023

TylerAPfledderer Aug 8, 2023

TylerAPfledderer Aug 8, 2023

pettinarip Aug 8, 2023

TylerAPfledderer Aug 8, 2023

nloureiro commented Aug 14, 2023

TylerAPfledderer commented Aug 17, 2023

TylerAPfledderer commented Aug 21, 2023 •

edited

Loading

TylerAPfledderer commented Aug 22, 2023

nloureiro Sep 4, 2023

TylerAPfledderer Sep 4, 2023

pettinarip left a comment

		@@ -0,0 +1,48 @@
		import { defineStyle, defineStyleConfig } from "@chakra-ui/react"

		const sizes = {

feat: Update Text and Heading Themes for New DS #10606

feat: Update Text and Heading Themes for New DS #10606

Conversation

TylerAPfledderer commented Jul 4, 2023

Description

Related Issue

gatsby-cloud bot commented Jul 4, 2023 • edited Loading

✅ ethereum-org-website-dev deploy preview ready

TylerAPfledderer commented Jul 7, 2023 • edited Loading

pettinarip commented Jul 27, 2023

Choose a reason for hiding this comment

TylerAPfledderer Jul 31, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nloureiro commented Aug 14, 2023

TylerAPfledderer commented Aug 17, 2023

TylerAPfledderer commented Aug 21, 2023 • edited Loading

NOTE

TylerAPfledderer commented Aug 22, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pettinarip left a comment

Choose a reason for hiding this comment

gatsby-cloud bot commented Jul 4, 2023 •

edited

Loading

TylerAPfledderer commented Jul 7, 2023 •

edited

Loading

TylerAPfledderer Jul 31, 2023 •

edited

Loading

TylerAPfledderer commented Aug 21, 2023 •

edited

Loading