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

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

Merged
Merged
Show file tree
Hide file tree
Changes from 10 commits
Commits
Show all changes
26 commits
Select commit Hold shift + click to select a range
9511d07
feat(theme/Heading): add new theme for DS
TylerAPfledderer Jul 4, 2023
1585ecc
feat(theme/Text): update theme for new DS
TylerAPfledderer Jul 5, 2023
22b8d0c
refactor(theme/Link): add sizes from Text theme and add Link to Text …
TylerAPfledderer Jul 5, 2023
de25c36
refactor(theme/text): set global font size and add body copy story
TylerAPfledderer Jul 6, 2023
97ce437
refactor(theme/styles): remove global styles for body copy and headings
TylerAPfledderer Jul 6, 2023
adf11e6
refactor(theme/Button): update Button component font size
TylerAPfledderer Jul 7, 2023
3ccf261
Merge remote-tracking branch 'origin/dev' into feat/heading-text-them…
TylerAPfledderer Jul 7, 2023
22e3cd4
Merge remote-tracking branch 'origin/dev' into feat/heading-text-them…
TylerAPfledderer Jul 16, 2023
db48b20
Merge remote-tracking branch 'origin/dev' into feat/heading-text-them…
TylerAPfledderer Jul 19, 2023
136fd28
Merge remote-tracking branch 'origin/dev' into feat/heading-text-them…
TylerAPfledderer Jul 19, 2023
e0173dc
Merge remote-tracking branch 'origin/dev' into feat/heading-text-them…
TylerAPfledderer Aug 2, 2023
36e303b
feat: add OldText component
TylerAPfledderer Aug 3, 2023
7b7e05c
refactor(theme/Text): update size token names to match updated DS
TylerAPfledderer Aug 8, 2023
b1f02f0
refactor(theme/styles): shrink default body copy for desktop
TylerAPfledderer Aug 8, 2023
c3a38a5
Merge remote-tracking branch 'origin/dev' into feat/heading-text-them…
TylerAPfledderer Aug 17, 2023
7d7eeba
fix(Card): update text size and spacing
TylerAPfledderer Aug 22, 2023
4b35120
fix(Slider): update text sizing and spacing
TylerAPfledderer Aug 22, 2023
74e5dc8
fix(Link.stories): add back spacing for link list
TylerAPfledderer Aug 22, 2023
cf974b8
refactor code to use the new OldText component
pettinarip Aug 22, 2023
a500d33
Merge branch 'dev' into feat/heading-text-theme-new-ds
pettinarip Aug 30, 2023
7f26b77
create new OldHeading to hold old styles and refactor codebase to use it
pettinarip Aug 30, 2023
20d98a2
fixes
pettinarip Sep 1, 2023
f2d3572
use chakra components
pettinarip Sep 1, 2023
6e56568
more fixes
pettinarip Sep 1, 2023
7fbab53
migrate all html headings to use Chakra Heading component
pettinarip Sep 1, 2023
af14c1b
removed a extra space
nloureiro Sep 5, 2023
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 7 additions & 1 deletion .storybook/i18next.ts
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,13 @@ export const baseLocales = {
}

// Only i18n files named in this array are being exposed to Storybook. Add filenames as necessary.
const ns = ["common", "page-about", "page-upgrades", "page-developers-index"]
const ns = [
"common",
"page-about",
"page-index",
"page-upgrades",
"page-developers-index",
]
const supportedLngs = Object.keys(baseLocales)

/**
Expand Down
1 change: 1 addition & 0 deletions src/@chakra-ui/gatsby-plugin/components/Button.ts
Original file line number Diff line number Diff line change
Expand Up @@ -94,6 +94,7 @@ const sizes = {
md: {
py: "2 !important",
px: "4 !important",
fontSize: "md",
[ICON_SELECTOR]: {
fontSize: "2xl",
},
Expand Down
39 changes: 39 additions & 0 deletions src/@chakra-ui/gatsby-plugin/components/Heading.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,39 @@
import { defineStyle, defineStyleConfig } from "@chakra-ui/react"
import { headingDefaultTheme } from "./components.utils"

const { sizes: defaultSizes } = headingDefaultTheme

const lineHeightScale = {
"4xl": "6xs",
"3xl": ["5xs", null, "6xs"],
"2xl": ["5xs", null, "4xs"],
xl: ["3xs", null, "2xs"],
lg: ["3xs", null, "2xs"],
md: "xs",
sm: "base",
xs: "base",
}

/*
* Instead of rewriting the entire sizes object, take the existing value from the
* default theme and replace the lineHeight values.
*/
const sizes = Object.entries(defaultSizes || {}).reduceRight(
(acc, [key, value]) => {
return {
...acc,
[key]: defineStyle({
...value,
lineHeight: lineHeightScale[key],
}),
}
},
{
...defaultSizes,
}
)

export const Heading = defineStyleConfig({
...headingDefaultTheme,
sizes,
})
3 changes: 3 additions & 0 deletions src/@chakra-ui/gatsby-plugin/components/Link.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
import { defineStyleConfig } from "@chakra-ui/react"
import components from "."
import { defineMergeStyles, linkDefaultTheme } from "./components.utils"
import { Text } from "./Text"

export const Link = defineStyleConfig({
baseStyle: defineMergeStyles(linkDefaultTheme.baseStyle, {
Expand All @@ -13,4 +15,5 @@ export const Link = defineStyleConfig({
outline: "auto",
},
}),
sizes: Text.sizes,
})
48 changes: 48 additions & 0 deletions src/@chakra-ui/gatsby-plugin/components/Text.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,48 @@
import { defineStyle, defineStyleConfig } from "@chakra-ui/react"

const sizes = {
Copy link
Member

Choose a reason for hiding this comment

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

@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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@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)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@pettinarip this is now updated in the PR

"7xl": defineStyle({
fontSize: "6xl",
lineHeight: "4xs",
}),
"6xl": defineStyle({
fontSize: "5xl",
lineHeight: "4xs",
}),
"5xl": defineStyle({
fontSize: "4xl",
lineHeight: "sm",
}),
"4xl": defineStyle({
fontSize: "3xl",
lineHeight: "sm",
}),
"3xl": defineStyle({
fontSize: "2xl",
lineHeight: "sm",
}),
"2xl": defineStyle({
fontSize: "xl",
lineHeight: "sm",
}),
xl: defineStyle({
fontSize: "lg",
lineHeight: "base",
}),
lg: defineStyle({
fontSize: "md",
lineHeight: "base",
}),
md: defineStyle({
fontSize: "sm",
lineHeight: "base",
}),
sm: defineStyle({
fontSize: "xs",
lineHeight: "base",
}),
}

export const Text = defineStyleConfig({
sizes,
})
5 changes: 4 additions & 1 deletion src/@chakra-ui/gatsby-plugin/components/index.ts
Original file line number Diff line number Diff line change
@@ -1,12 +1,14 @@
import { Avatar } from "./Avatar"
import { Badge } from "./Badge"
import { Button } from "./Button"
import { Heading } from "./Heading"
import { Link } from "./Link"
import { Tag } from "./Tag"
import { Modal } from "./Modal"
import { Checkbox } from "./Checkbox"
import { Progress } from "./Progress"
import { Tabs } from "./Tabs"
import { Text } from "./Text"
import { Radio } from "./Radio"
import { Select } from "./Select"
import { Switch } from "./Switch"
Expand Down Expand Up @@ -39,7 +41,7 @@ export default {
Drawer: drawerDefaultTheme,
Form: formDefaultTheme,
FormLabel: formLabelDefaultTheme,
Heading: headingDefaultTheme,
Heading,
Input,
Link,
Menu: menuDefaultTheme,
Expand All @@ -52,4 +54,5 @@ export default {
Table: tableDefaultTheme,
Tabs,
Tag,
Text,
}
7 changes: 4 additions & 3 deletions src/@chakra-ui/gatsby-plugin/foundations/typography.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,9 +6,10 @@ const typography = {
},

lineHeights: {
"5xs": 1.1,
"4xs": 1.15,
"3xs": 1.2,
"6xs": 1.1,
"5xs": 1.15,
"4xs": 1.2,
"3xs": 1.25,
"2xs": 1.35,
xs: 1.4,
sm: 1.5,
Expand Down
64 changes: 7 additions & 57 deletions src/@chakra-ui/gatsby-plugin/styles.ts
Original file line number Diff line number Diff line change
@@ -1,7 +1,5 @@
import { mode } from "@chakra-ui/theme-tools"

import { lightTheme as oldTheme } from "../../theme"

const styles = {
global: (props) => ({
/**
Expand All @@ -17,16 +15,18 @@ const styles = {
// TODO: when we have Chakra v2, this should be done by overriding the
// native Chakra semantic tokens
bg: mode("white", "gray.700")(props),
lineHeight: "1.6rem",
lineHeight: "base",
fontSize: ["sm", null, "lg"],
},
p: {
_last: {
mb: 0,
},
},
a: {
color: "primary.base",
textDecoration: "underline",
},
// should be replace with https://chakra-ui.com/docs/components/text
p: {
margin: "0px 0px 1.45rem",
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@TylerAPfledderer TylerAPfledderer Jul 31, 2023

Choose a reason for hiding this comment

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

@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.

Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

Correct.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@pettinarip I've pushed this new component here.

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds good!

},
// should be replace with https://chakra-ui.com/docs/components/list
"ul, ol": {
margin: "0px 0px 1.45rem 1.45rem",
Expand Down Expand Up @@ -56,59 +56,9 @@ const styles = {
"li *:last-child": {
"margin-bottom": "0",
},
"p *:last-child": {
"margin-bottom": "0",
},
"li > p": {
"margin-bottom": "calc(1.45rem / 2)",
},
// should be replace by the usage of https://chakra-ui.com/docs/components/heading
// also, the media queries defined on each of these heading tags are bearly used
"h1,h2,h3,h4,h5,h6": {
margin: "2rem 0",
lineHeight: 1.4,
scrollMarginTop: "navHeight",
scrollSnapMargin: "navHeight",
},
h1: {
fontSize: "3rem",
fontWeight: 700,
[`@media (max-width: ${oldTheme.breakpoints.m})`]: {
fontSize: "2.5rem",
},
},
h2: {
fontSize: "2rem",
marginTop: "3rem",
fontWeight: 600,
[`@media (max-width: ${oldTheme.breakpoints.m})`]: {
fontSize: "1.5rem",
},
},
h3: {
fontSize: "1.5rem",
marginTop: "2.5rem",
fontWeight: 600,
[`@media (max-width: ${oldTheme.breakpoints.m})`]: {
fontSize: "1.25rem",
},
},
h4: {
fontSize: "1.25rem",
fontWeight: 500,
[`@media (max-width: ${oldTheme.breakpoints.m})`]: {
fontSize: "1rem",
},
},
h5: {
fontSize: "1rem",
fontWeight: 450,
},
h6: {
fontSize: "0.9rem",
fontWeight: 400,
textTransform: "uppercase",
},
// Anchor tag styles
// Selected specifically for mdx rendered side icon link
".header-anchor": {
Expand Down
98 changes: 98 additions & 0 deletions src/components/BaseStories/Heading.stories.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,98 @@
import * as React from "react"
import {
Box,
Flex,
Heading as HeadingComponent,
HeadingProps,
Stack,
VStack,
} from "@chakra-ui/react"
import { Meta, StoryObj } from "@storybook/react"
import Translation from "../Translation"

type HeadingType = typeof HeadingComponent

const meta = {
title: "Atoms / Typography / Heading",
component: HeadingComponent,
parameters: {
layout: null,
},
decorators: [
(Story) => (
<Flex align="center" minH="100vh">
<Story />
</Flex>
),
],
} satisfies Meta<HeadingType>

export default meta

type Story = StoryObj<typeof meta>

const headingScale: Array<HeadingProps> = [
{
as: "h1",
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@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.

size: "4xl",
},
{
as: "h1",
size: "3xl",
},
{
as: "h1",
size: "2xl",
},
{
// No props as the default is `h2` with size `xl
},
{
as: "h3",
size: "lg",
},
{
as: "h4",

size: "md",
},
{
as: "h5",
size: "sm",
},
{
as: "h6",
size: "xs",
},
]

export const Heading: Story = {
args: {
children: <Translation id="page-index-title" />,
},
render: (args) => (
<VStack w="full">
<Box>
Adjust the viewport to below "md" to see the font size and line height
change
</Box>
<Stack>
{headingScale.map((obj, idx) => (
<Flex key={idx} gap="6">
<HeadingComponent
as="span"
flex="1"
textAlign="right"
size={obj.size}
>
{obj.size || "xl"}
</HeadingComponent>
<HeadingComponent flex="3" {...obj}>
{args.children}
</HeadingComponent>
</Flex>
))}
</Stack>
</VStack>
),
}
Loading