[WIP] New Heading API proposal by emplums · Pull Request #780 · primer/react

emplums · 2020-04-27T23:15:03Z

This PR implements a new Heading API proposal that I'd love to get some feedback on!

The Problem

Currently in Primer Components, we don't really set font size defaults for different heading sizes. The default h2 gets a heading size set, burt if you change the heading to h3 the font size remains the same unless you update it yourself manually using the fontSize prop. In order for the Heading component to be a useful design system tool, I think it should provide some font size defaults for different heading levels.

The Solution

Create separate components for each heading level. H1, H2, H3, H4, H5, and H6. This nudges the user to think a little more about what heading level they're using, and allows us to set default font sizes (and font sizes for responsive breakpoints!) for each level.

Drawbacks

This is obviously a large breaking change because we'd be deprecating Heading. I considered keeping Heading and using the as prop but it was finicky (see next section for more details). IMO, having a different component for each heading size feels like the best API but we have been making a lot of breaking changes lately so not 1000000% sure it's worth it.

Other Approaches Considered

I considered keeping Heading and using the as prop to dynamically set the font size. This worked okay but it was fragile and would break if someone were to pass in a styled component to the as prop.

This is basically what I tried:

const getFontSize = (props) => {
  const fontSizeMap = {
    h1: get('headingSizes.h1')(props),
    h2: get('headingSizes.h2')(props),
    h3: get('headingSizes.h3')(props),
    h4: get('headingSizes.h4')(props),
    h5: get('headingSizes.h5')(props),
    h6: get('headingSizes.h6')(props),
  }
  return fontSizeMap[props.as]
}

const Heading = styled.h2`
  font-size: ${props => getFontSize(props)};
  font-weight: ${get('fontWeights.bold')};
  margin: 0;
  ${TYPOGRAPHY} ${COMMON};
`

It just didn't feel great and also would have meant we'd need to add a media query in for the mobile font sizes.

Deprecating Heading

I propose that we deprecate the Heading component gradually in favor of using H1, H2, H3, H4, H5, H6. I'll keep it in for now + add a deprecation notice, but let's remove it after a month or two ☮️

I added a Deprecated component that will allow you to throw a console warning whenever a component that it wraps is used.

const Heading = (props) => {(
  <Deprecated componentName="Heading" message="Use the H1, H2, H3, H4, H5, H6 components instead">
    <StyledHeading {...props}/>
  </Deprecated>
)}

Notes

I wanted to add a headingSizes and mobileHeadingSizes to the theme and pull from those values so that they could be overridden if needed, but styled system expects to pull from the fontSizes object in the value for the fontSize prop, and it would be a huuuuge breaking change to add named keys to the fontSize theme object (bc it's used in so many places), so I ended up just hard coding these values. If we decide to update the heading font sizes in the future it will be easy enough to update in this one component, and if someone would like to use a different heading size scale, they can create their own heading components. I think this makes the most sense since the Heading component is basically just setting the correct font size for each heading level anyways. jk i figured it out thanks 2 @BinaryMuse

vercel · 2020-04-27T23:15:06Z

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/primer/primer-components/ov9e9os1w
✅ Preview: https://primer-components-git-heading.primer.now.sh

BinaryMuse · 2020-04-28T17:23:20Z

and it would be a huuuuge breaking change to add named keys to the fontSize theme object

🤔 Can we do both?

const fontSizes = [...]
Object.assign(fontSizes, {
  name: value,
  another: value
})

emplums · 2020-04-29T16:37:01Z

@BinaryMuse oooo good idea duh 😂

docs/content/Heading.md

emplums · 2020-04-29T17:55:02Z

index.d.ts

+declare module '@primer/components/src/Position' {
+  import {Relative, Absolute, Sticky, Fixed} from '@primer/components'
+  export {Relative, Absolute, Sticky, Fixed}
+}


@dmarcey do these modules look right? previously we were exporting each one from @primer/components/src/Relative, etc but that's incorrect because there's not actually a file there (they are all exported from on Position file) similar to how we do it for Heading

NB: It’s possible to combine the import and export into a single line:

export { Relative, Absolute, Sticky, Fixed } from "@primer/components";

That looks like it should work! @smockle 's suggestion seems like it simplifies things a little bit too.

emplums · 2020-04-29T18:32:38Z

src/Heading.js

 }
+//
+
+const HeadingInternal = styled.h2`


This is basically the same as Heading but without a default font size. All of the stuff above this line will get removed when we deprecate Heading

BinaryMuse · 2020-04-29T18:34:26Z

src/utils/Deprecated.js

+const Deprecated = ({componentName, message, children}) => {
+  console.warn(`WARNING! ${componentName} will be deprecated soon. ${message}`)
+  return children
+}
+


I really really like this! Thoughts about adding a since prop that specifies the version that the deprecation was created in? Could also be useful for us to grep through the codebase and see which deprecations are ready to be removed.

yeah that's a great idea! I'll add it!

I added a version prop instead, for which version the component is planning to be deprecated in. I'm assuming the term deprecated to mean completely removed so this is what I was thinking the process would be:

When you are getting ready to remove a component or prop, first wrap the component in Deprecated and specify which future version the component will be completely removed in.

If there is a migration strategy (in this case it's to use H1, etc instead), that new API should be available when the deprecation notice is added so the users can start migrating immediately. In other words, in most cases when you are adding a new component or prop that replaces an old component/prop, keep both around for a period of time to allow folks to migrate. It might not always make sense to do this. I could see situations where we want to remove a component completely without a replacement or with a replacement that lives outside the library (for instance, removing StyledOcticon in favor of the external @primer/styled-octicons library).

Keep the deprecation warning + old component around for a specified period of time. In this case, since we're going to ship 19.0.0 very soon, I set it for 20.0.0 so that we can give folks time to see the warning.

When shipping a major release, search the codebase for version=<VERSION ABOUT TO BE RELEASED>. Completely remove the components wrapped in Deprecated and all related type definitions, exports, docs, tests etc.

What do you think? Maybe the component name should be DeprecationWarning instead?

I think "deprecated" is commonly used as "still available but in the process of being changed or removed;" if an API is completely removed, it's no longer deprecated.

That said, I'm okay with using a prop to note which version we plan on removing the feature in. I guess it's not all that important to know when the API was deprecated. The only downside is that time between major releases is not always consistent.

👍 on DeprectionWarning as a name

BinaryMuse

Liking the direction you went with this. A few comments below.

src/Heading.js

BinaryMuse · 2020-04-29T18:37:31Z

src/Heading.js

+function withHeading(level) {
+  const WithHeading = props => (
+    <HeadingInternal
+      as={level}


Curious how this behaves if someone does <H1 as={SomeStyledComponent} ...

Good point 🤔 Since it's such a simple component I'm not sure if it's worth the pain to allow people to pass in a styled component or anything else to the as prop. If folks really need to add some styles to the headings, they could do:

const CustomHeading = styled(H1)` // custom styles `

I think allowing the as prop on this component would open it up for folks to change the heading level to to get a different font size like so:

<H1 as='h2'>Heading</H1>

which we want to discourage in favor of this (to encourage proper semantics/a11y:

<H2 fontSize='headingSizes.h1' />

People could even go as far as:

<H1 as='p'>HEADING!</H1>

which we really wouldn't want.

That sounds good to me, although the TypeScript and propTypes will still list as as a valid prop

BinaryMuse · 2020-04-29T18:38:37Z

src/Heading.js

+export const H1 = withHeading('h1')
+export const H2 = withHeading('h2')
+export const H3 = withHeading('h3')
+export const H4 = withHeading('h4')
+export const H5 = withHeading('h5')
+export const H6 = withHeading('h6')


If Heading is being deprecated then it should probably still be exported as the default export so that the imports don't break. The TS module will need to be updated accordingly.

Does it matter though since we're exporting everything as named exports in index.js? We aren't importing Heading in any other component files so we shouldn't be relying on the default export anywhere

The previous module definition for src/Heading was:

declare module '@primer/components/src/Heading' { import {Heading} from '@primer/components' export default Heading }

so if we change Heading.js so that Heading isn't being exported as default, that's a breaking change for this TypeScript module.

Also #789 will make individual modules (JS) requireable, so we will need to stick to an export contract for those.

`

Co-Authored-By: Michelle Tilley <binarymuse@github.com>

dmarcey · 2020-04-30T12:17:57Z

index.d.ts

-  import {Heading} from '@primer/components'
-  export default Heading
+  import {Heading, H1, H2, H3, H4, H5, H6} from '@primer/components'
+  export {Heading, H1, H2, H3, H4, H5, H6}


Can you do

export default Heading; export {Heading, H1, H2, H3, H4, H5, H6}

for TS compat with the deprecated 'Heading` default export?

emplums · 2020-05-11T21:18:06Z

Closing for now

new Heading API proposal

c83d8c8

emplums requested review from BinaryMuse and broccolini April 27, 2020 23:15

add heading sizes to the theme

ce9a072

vercel bot temporarily deployed to Preview April 29, 2020 17:18 Inactive

Update Heading.js

1c273d6

vercel bot temporarily deployed to Preview April 29, 2020 17:26 Inactive

refactor

de59464

vercel bot had a problem deploying to Preview April 29, 2020 17:40 Failure

emplums commented Apr 29, 2020

View reviewed changes

docs/content/Heading.md Show resolved Hide resolved

Update index.d.ts

7aab53f

vercel bot had a problem deploying to Preview April 29, 2020 17:52 Failure

emplums commented Apr 29, 2020

View reviewed changes

typo

73556cd

vercel bot had a problem deploying to Preview April 29, 2020 18:05 Failure

keep Heading around for now

4b42982

vercel bot temporarily deployed to Preview April 29, 2020 18:13 Inactive

lint

36990d6

vercel bot temporarily deployed to Preview April 29, 2020 18:14 Inactive

emplums added the minor release new features label Apr 29, 2020

add deprecation warning

6250aae

vercel bot had a problem deploying to Preview April 29, 2020 18:25 Failure

emplums commented Apr 29, 2020

View reviewed changes

BinaryMuse reviewed Apr 29, 2020

View reviewed changes

update docs

a0a220d

vercel bot had a problem deploying to Preview April 29, 2020 18:36 Failure

lint

9e746bd

vercel bot requested a deployment to Preview April 29, 2020 18:38 Abandoned

BinaryMuse suggested changes Apr 29, 2020

View reviewed changes

add version prop to deprecated

def7303

`

vercel bot had a problem deploying to Preview April 29, 2020 18:40 Failure

Update src/Heading.js

db6b459

Co-Authored-By: Michelle Tilley <binarymuse@github.com>

vercel bot had a problem deploying to Preview April 29, 2020 18:54 Failure

Emily Plummer added 2 commits April 29, 2020 12:17

don't allow as prop

2cc38f1

Merge branch 'heading' of github.com:primer/components into heading

eeaa43f

vercel bot temporarily deployed to Preview April 29, 2020 21:23 Inactive

emplums changed the title ~~New Heading API proposal~~ [WIP] New Heading API proposal Apr 29, 2020

dmarcey reviewed Apr 30, 2020

View reviewed changes

BinaryMuse mentioned this pull request May 1, 2020

Release v19.0.0 #764

Merged

24 tasks

emplums closed this May 11, 2020

emplums deleted the heading branch September 16, 2020 21:26

Conversation

emplums commented Apr 27, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

The Problem

The Solution

Drawbacks

Other Approaches Considered

Deprecating Heading

Notes

Uh oh!

vercel bot commented Apr 27, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

BinaryMuse commented Apr 28, 2020

Uh oh!

emplums commented Apr 29, 2020

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

emplums Apr 29, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

BinaryMuse left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

emplums Apr 29, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

BinaryMuse Apr 29, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

emplums commented May 11, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

emplums commented Apr 27, 2020 •

edited

Loading

vercel bot commented Apr 27, 2020 •

edited

Loading

emplums Apr 29, 2020 •

edited

Loading

emplums Apr 29, 2020 •

edited

Loading

BinaryMuse Apr 29, 2020 •

edited

Loading