Skip to content

Conversation

@BinaryMuse
Copy link
Contributor

@BinaryMuse BinaryMuse commented May 5, 2020

This builds on the great work by @tiaanduplessis in #752 (thank you @tiaanduplessis!) and pulls in all the changes from #789 (pending merge into release-19.0.0, will update this PR base branch when that lands).

I opted for this style for defining variants based on the variant docs and notes on the deprecated API. This style should allow us to set values based on the theme but also allow overriding the variant definitions when a user passes their own theme.

Closes #752
Closes #696

Release notes

  • StateLabel's small prop is deprecated in favor of a new variant prop. Use variant="small" to achieve the same result. We plan to remove the small prop in version 20.

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

@vercel
Copy link

vercel bot commented May 5, 2020

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/gwpng274w
✅ Preview: https://primer-components-git-mkt-more-state-label-work.primer.now.sh

@vercel vercel bot temporarily deployed to Preview May 5, 2020 21:35 Inactive
@vercel vercel bot temporarily deployed to Preview May 5, 2020 21:38 Inactive
@vercel vercel bot temporarily deployed to Preview May 5, 2020 21:39 Inactive
@vercel vercel bot temporarily deployed to Preview May 5, 2020 21:54 Inactive
@BinaryMuse BinaryMuse requested a review from emplums May 5, 2020 21:59
@BinaryMuse
Copy link
Contributor Author

This is ready for review 👀

@BinaryMuse BinaryMuse mentioned this pull request May 5, 2020
24 tasks
@BinaryMuse
Copy link
Contributor Author

It looks like there's an issue with Octicon; this code in StateLabel:

  const octiconProps = variant === 'small' ? {width: '1em'} : {}
  return (
      // ...
      {status && <StyledOcticon mr={1} {...octiconProps} icon={octiconMap[status] || Question} />}

causes Octicon to fail prop type validation because "1em", a string, is not expected. As a result, the Octicon sets its height to NaN.

/cc @colebemis

Copy link

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

Left a few suggestions for how to deal with the theme!

Co-authored-by: Emily <emplums@github.com>
@vercel vercel bot temporarily deployed to Preview May 5, 2020 22:47 Inactive
Co-authored-by: Emily <emplums@github.com>
@vercel vercel bot temporarily deployed to Preview May 7, 2020 00:17 Inactive
Co-authored-by: Emily <emplums@github.com>
@vercel vercel bot temporarily deployed to Preview May 7, 2020 00:17 Inactive
@vercel vercel bot temporarily deployed to Preview May 7, 2020 00:20 Inactive
@vercel vercel bot temporarily deployed to Preview May 7, 2020 00:25 Inactive
@BinaryMuse BinaryMuse changed the base branch from mkt/bundle-up to release-19.0.0 May 7, 2020 00:45
@BinaryMuse BinaryMuse merged commit d4874f0 into release-19.0.0 May 7, 2020
@BinaryMuse BinaryMuse deleted the mkt/more-state-label-work branch May 7, 2020 00:46
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