Skip to content

Conversation

@emplums
Copy link

@emplums emplums commented Nov 30, 2018

Refactors all of our components to use styled-components and removes our system-component custom setup 🙌

Notes:

  • I'm removing the OcticonButton component because it's essentially a <Link><Octicon/><Link>

To Do

  • Add is to all propTypes objects that use it
  • Add theme to all propTypes
  • Double check default theme doesn't ever override custom theme (especially when spreading theme out of proto component)
  • Add test that makes sure default theme is applied to every component
  • Make sure every component has a test for system props
  • . Create a codemod
  • add babel plugin [wait to do this after removing Primer CSS]
  • fix PointerBox

@vercel
Copy link

vercel bot commented Nov 30, 2018

This pull request is automatically deployed with Now.
To access deployments, click Details below or on the icon next to each push.

@emplums emplums mentioned this pull request Dec 3, 2018
5 tasks
Copy link
Contributor

@shawnbot shawnbot 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 EPIC, @emplums. Nice work! 👏

@shawnbot
Copy link
Contributor

I'm sure you're aware that there are a ton of console errors in the test suite. Let me know if I can help you work through some of those.

@shawnbot shawnbot mentioned this pull request Dec 18, 2018
Copy link
Contributor

@mxstbr mxstbr left a comment

Choose a reason for hiding this comment

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

Gave the diff a quick read, this looks amazing! 👏👏👏 Good job! 💯

src/Text.js Outdated
Text.propTypes = {
...TYPOGRAPHY.propTypes,
...COMMON.propTypes,
is: PropTypes.oneOfType([PropTypes.string, PropTypes.func]),
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be PropTypes.node instead of PropTypes.func? As far as I can tell the way the is prop is implemented should mean any element or component can be passed, not just function components.

Copy link
Author

Choose a reason for hiding this comment

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

Good catch!

@emplums emplums mentioned this pull request Dec 20, 2018
5 tasks
@emplums emplums changed the base branch from master to release-q4-santa December 20, 2018 21:49
@emplums
Copy link
Author

emplums commented Dec 20, 2018

Going to merge this into the release branch and do a separate docs PR!

@emplums emplums merged commit 36461ab into release-q4-santa Dec 20, 2018
@emplums emplums deleted the big-refactor branch December 20, 2018 22:53
Copy link
Contributor

@mxstbr mxstbr left a comment

Choose a reason for hiding this comment

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

🎉🎉🎉🎉🎉🎉🎉🎉🎉🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants