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

React Octicons! #222

Merged
merged 99 commits into from
Jun 29, 2018
Merged

React Octicons! #222

merged 99 commits into from
Jun 29, 2018

Conversation

shawnbot
Copy link
Contributor

@shawnbot shawnbot commented Jun 8, 2018

This is a rehash of #196 introducing a React component API that looks like this:

import Octicon, {Zap} from '@github/react-octicons'

export default () => <Octicon><Zap /></Octicon>
// or
export default () => <Octicon icon={Zap} />

With credit to @hiendv for the inspiration in #196 (comment) 🍻

Note that there's one wild assumption in the generated per-icon components here, and it's that the SVG from the octicons package is valid XML, which is what allows us to use it in JSX without munging it. I've checked the generated icon component JS into git so that we can look at it without the module having to publish.

TODO

  • Add new tests for React Octicons
  • Get tests passing
  • Update README.md with new API (see rendered)
  • Deploy test site

const given = width ? 'width' : 'height'
const computed = given === 'width' ? 'height' : 'width'
attrs[given] = width || height || sizeMap[size] || size
attrs[computed] = attrs[given] * (dims[computed] / dims[given])
Copy link
Contributor Author

Choose a reason for hiding this comment

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

If both the width and height aren't provided, we compute the width or the height — depending on whether they provide width, height, or size (which can be either a number or a string that's mapped via sizeMap).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note: the parens on that last line above are a linting requirement. * and / have the same operator precedence, so I think the rule that wants one expression grouped exists to help us be more explicit about what it's doing.

height: PropTypes.number,
icon: PropTypes.func,
size: PropTypes.oneOfType([PropTypes.number, PropTypes.oneOf(Object.keys(sizeMap))]),
verticalAlign: PropTypes.oneOf(['middle', 'text-bottom', 'text-top', 'top']),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is there a good reason not to allow bottom here?

@@ -5,9 +5,11 @@
"homepage": "https://octicons.github.com",
"author": "GitHub, Inc.",
"license": "MIT",
"main": "index.js",
"main": "build/index.js",
"module": "index.js",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was wrong for me to include. I removed it in 02a007a.

"presets": ["env", "stage-0", "react"],
"env": {
"production": {
"presets": ["next/babel"]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is used when we call NODE_ENV=production next in the start run-script.

@@ -0,0 +1 @@
src/icons/*.js
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Don't lint the generated files. 🙈

@@ -0,0 +1 @@
8
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is for development. I've added the equivalent to engines.node in the package.json as well.

@shawnbot shawnbot changed the title [WIP] React Octicons! React Octicons! Jun 29, 2018
@shawnbot
Copy link
Contributor Author

FYI you can see what the separate ESM and UMD builds look like in the alpha release of b61e03c:

  • ESM (with import and export statements)
  • UMD (with require() statements)

@shawnbot shawnbot changed the base branch from master to release-7.4.0 June 29, 2018 18:28
@shawnbot shawnbot merged commit 50a65b2 into release-7.4.0 Jun 29, 2018
@shawnbot shawnbot deleted the reocticons branch June 29, 2018 18:28
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