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

Add icon-specific className to React default props #453

Merged
merged 7 commits into from
Feb 23, 2021
Merged

Add icon-specific className to React default props #453

merged 7 commits into from
Feb 23, 2021

Conversation

FloEdelmann
Copy link
Contributor

In GitHub, all the icons have a class octicon-${key}, e.g. octicon-arrow-both for the ArrowBothIcon. This PR adds this class to the React components' default props:

  function ArrowBothIcon(props) {
    // …
    return <svg {...getSvgProps({...props, svgDataByHeight})} />
  }

  ArrowBothIcon.defaultProps = {
-   className: 'octicon',
+   className: 'octicon octicon-arrow-both',
    size: 16,
    verticalAlign: 'text-bottom'
  }

@vercel
Copy link

vercel bot commented Jun 14, 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/octicons/4pCMWDx4tp5mFvQM2KZ99fymbfmF
✅ Preview: https://octicons-git-fork-floedelmann-class-names-primer.vercel.app

@fregante
Copy link
Contributor

I think this makes sense since by default it also has the octicon class as well

@FloEdelmann
Copy link
Contributor Author

@colebemis @ashygee Could you please have a quick look at this PR?

@vercel vercel bot temporarily deployed to Preview September 20, 2020 14:47 Inactive
@ashygee
Copy link
Contributor

ashygee commented Sep 28, 2020

@FloEdelmann Thanks for this and sorry for the delay! I've assigned this to @colebemis and we'll review this week. Apologies for not seeing this sooner.

@vercel vercel bot temporarily deployed to Preview September 28, 2020 16:37 Inactive
@vercel vercel bot temporarily deployed to Preview October 5, 2020 21:10 Inactive
@FloEdelmann
Copy link
Contributor Author

@colebemis Sorry to bother you again, but given this is literally only one changed line, I don't understand why you still didn't review. Having that extra class name would be really helpful, e.g. in refined-github/refined-github#3227.

@fregante
Copy link
Contributor

No rush Flo 🤗

@yakov116
Copy link

@colebemis friendly bump

@yakov116
Copy link

@colebemis @ashygee its almost 7 months since this PR has been opened and 3 releases. Any chance this can be reviewed?

@vercel vercel bot temporarily deployed to Preview February 23, 2021 16:04 Inactive
@vercel vercel bot temporarily deployed to Preview February 23, 2021 16:08 Inactive
Copy link
Contributor

@colebemis colebemis left a comment

Choose a reason for hiding this comment

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

Looks great! Sorry for the delay.

We'll ship this in our next release.

@vercel vercel bot temporarily deployed to Preview February 23, 2021 18:29 Inactive
@colebemis colebemis changed the base branch from master to release-12.1.0 February 23, 2021 19:34
@colebemis colebemis merged commit ae719f9 into primer:release-12.1.0 Feb 23, 2021
@colebemis colebemis mentioned this pull request Feb 23, 2021
2 tasks
@FloEdelmann FloEdelmann deleted the class-names branch February 24, 2021 06:57
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.

None yet

5 participants