Skip to content

Heading update (new default level and updated docs)#716

Merged
BinaryMuse merged 4 commits intoprimer:majorfrom
taylorcjohnson:taylorcjohnson-heading-update
Apr 1, 2020
Merged

Heading update (new default level and updated docs)#716
BinaryMuse merged 4 commits intoprimer:majorfrom
taylorcjohnson:taylorcjohnson-heading-update

Conversation

@taylorcjohnson
Copy link
Copy Markdown
Contributor

@taylorcjohnson taylorcjohnson commented Mar 16, 2020

This draft pull request attempts to address the comments mentioned in issue #694:

  1. Change Heading to default to h2 so users don't accidentally use multiple h1s
  2. Update the docs to be clear on how to choose a different heading level

The Heading component now defaults to using an h2 instead of h1. It also includes prop type validation for the as prop. This may not be necessary, but it attempts to restrict the html elements to only h1-h6 (please let me know if this is inappropriate). The docs page was also updated to make sure the user is aware that the default heading value is h2. It also updates the component props to include the as prop (similar to the TabNav component).

Finally, I wanted to verify that this is a major version change and that package.json should be updated accordingly (version 17.0.0 as of this pull request).

Closes #694

Screenshots

Please provide before/after screenshots for any visual changes

Merge checklist

  • Added or updated TypeScript definitions (index.d.ts) if necessary
  • Added/updated tests
  • Added/updated documentation
  • Tested in Chrome
  • Tested in Firefox
  • Tested in Safari
  • Tested in Edge

Take a look at the What we look for in reviews section of the contibuting guidelines for more infomation on how we review PRs.

Change default heading level to `h2` and include prop type validation for styled-component `as` prop.
Updates the Heading component documentation to note the new default heading level (`h2`) and how to change the heading level by passing another heading value to the `as` prop.
@vercel vercel bot temporarily deployed to Preview March 16, 2020 03:58 Inactive
@vercel
Copy link
Copy Markdown

vercel bot commented Mar 16, 2020

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

🔍 Inspect: https://zeit.co/primer/primer-components/78y77mxo0
✅ Preview: https://primer-compone-git-fork-taylorcjohnson-taylorcjohnson-he-e9b24e.primer.now.sh

@taylorcjohnson taylorcjohnson changed the title Taylorcjohnson heading update Heading update (new default level and updated docs) Mar 16, 2020
@taylorcjohnson taylorcjohnson marked this pull request as ready for review March 16, 2020 04:06
@auareyou auareyou requested a review from emplums March 16, 2020 07:52
@auareyou auareyou added the fr-skip Remove this from the Design Systems first responder list label Mar 16, 2020
@emplums emplums added the major release breaking changes label Mar 16, 2020
Copy link
Copy Markdown

@emplums emplums left a comment

Choose a reason for hiding this comment

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

This is great @taylorcjohnson thanks for picking this one up! Would you mind updating the TypeScript type definitions to only allow h1-h6 as valid as values? Other than that this is ready to go! 🙌

@emplums emplums changed the base branch from master to major March 16, 2020 23:58
@vercel vercel bot temporarily deployed to Preview March 17, 2020 04:09 Inactive
@taylorcjohnson
Copy link
Copy Markdown
Contributor Author

taylorcjohnson commented Mar 17, 2020

Thank you for taking a look @emplums! Apologies for missing the type definition 🤦‍♂️! I pushed an update, but would welcome extra review since I'm still new to TypeScript.

Additionally, it seems like I should update src/__tests__/Heading.js as well. The first test renders <h1> by default would now fail (Line 35).

There is also an invalid prop of i being used on line 97 to test the fontStyle prop - I could simply remove that expect block if i is definitely no longer a valid html element.

Update (4/1/2020): I updated the tests for this component. It checks to see if an <h2> is rendered by default and I removed the test that attempts to render a Heading as <i>.

@taylorcjohnson taylorcjohnson requested a review from emplums March 18, 2020 21:49
@emplums emplums mentioned this pull request Mar 31, 2020
8 tasks
@vercel vercel bot temporarily deployed to Preview April 1, 2020 21:52 Inactive
@BinaryMuse BinaryMuse merged commit 98af424 into primer:major Apr 1, 2020
@BinaryMuse
Copy link
Copy Markdown
Contributor

Tested manually and merged into release branch. Thanks again!

@taylorcjohnson
Copy link
Copy Markdown
Contributor Author

Thank you! Happy to help!

@taylorcjohnson taylorcjohnson deleted the taylorcjohnson-heading-update branch April 1, 2020 22:20
@BinaryMuse BinaryMuse mentioned this pull request Apr 14, 2020
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

fr-skip Remove this from the Design Systems first responder list major release breaking changes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Heading docs are misleading / confusing

4 participants