-
Notifications
You must be signed in to change notification settings - Fork 13
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
Allow Gridicons library to be imported by component #283
Changes from 27 commits
81ed4ee
d27ab4e
6946b70
e7b114c
5fbbef9
a82adb3
b32bf60
a9010c9
2c0e264
1344565
15357c6
df051c3
b4ad138
482b142
b4ca433
eb60b9c
dbb5a2c
64fb4ba
42fae14
84d9357
568cbde
d5963b9
d2d1478
bed88b4
fbd44f9
7001b5d
31ccaf0
27ca79f
c0d1361
a5bed97
6337624
37f871a
65ce3d9
0262b6b
b884c31
da5f24e
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -2,5 +2,6 @@ node_modules/ | |
/npm-debug.log | ||
.idea/ | ||
svg-min-react/ | ||
build/ | ||
cjs/ | ||
esm/ | ||
.DS_Store |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -2,7 +2,7 @@ | |
|
||
The [Calypso](https://github.com/Automattic/wp-calypso/) / [WordPress.com](https://wordpress.com) official icon set. | ||
|
||
## Using the Gridicon Component in your project: | ||
## Using the Gridicon Component in your project | ||
|
||
Note that this component requires [react](https://www.npmjs.com/package/react) to be installed in your project. If you don't want to use React, you can simply include the raw `.svg` files from the [`svg-min`](https://github.com/Automattic/gridicons/tree/master/svg-min) folder. | ||
|
||
|
@@ -16,6 +16,8 @@ npm install gridicons --save | |
|
||
#### Usage | ||
|
||
You can either import the whole iconset and decide at run-time which icon to use: | ||
|
||
``` | ||
import Gridicon from 'gridicons'; | ||
//... | ||
|
@@ -24,17 +26,29 @@ render() { | |
} | ||
``` | ||
|
||
Or import icons individually: | ||
|
||
``` | ||
import GridiconAddImage from 'gridicons/cjs/add-image'; | ||
//... | ||
render() { | ||
return <GridiconAddImage />; | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think you can drop the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I prefer to keep it: it instructs the consumer of the API how to use the component noting that in the general case you'll use the icon prop and if you import the individual component you don't. That's pretty clear to my eyes, but I'd rather err on the side of being obvious for new users. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What about making it a functional component to have a more viable/standalone example? 😁 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. E.g. import GridiconAddImage from 'gridicons/cjs/add-image';
//...
function MyComponent() {
return <GridiconAddImage />;
} There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I've had too much npm for today 😅 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think the render() is more accessible for folks who have just started with React, let's keep in it. :) |
||
``` | ||
|
||
We also support importing the untranspiled ECMASCript modules by using `gridicons/esm` instead of `gridicons/cjs`. | ||
|
||
#### Props | ||
|
||
* `icon`: String - the icon name. | ||
* `icon`: String - the icon name. This is ignored when importing icons individually. | ||
* `size`: Number - (default: 24) set the size of the icon. | ||
* `onClick`: Function - (optional) if you need a click callback. | ||
|
||
**Notes**: | ||
|
||
* The icon set is made to be used exactly at these pixel sizes: 12, 18, 24, 36, 48, 54, 72. | ||
* `gridicon-my-sites` as it's a small-size version of the WordPress logo, shouldn't be used larger than 36px. If you need to use the WordPress logo in larger sizes, see the [Social Logos project](https://github.com/Automattic/social-logos). | ||
|
||
* Individual icons are between 1K and 2K. If you use only a few, the recommended way of using the Gridicon library is to import them individually. At the moment, the main file containing the whole iconset sits at 92K. | ||
|
||
## Icon Set Style Guidelines | ||
|
||
|
@@ -72,7 +86,7 @@ Note that the icons in this set are tied to be used in [Calypso](https://github. | |
1. Switch to the branch (i.e. Pull Request) with the new icon. | ||
2. Review the SVG source of the new icons to make sure they are clean and readable. | ||
3. Check pixel sharpness: open in Illustrator (with "Pixel Preview") or Sketch (with "Show Pixels"), adjust if needed. | ||
4. Run `grunt` command from terminal. It will generate `svg-min`, React (`build`), `svg-sprite`, `pdf`, `php`, and `docs`. | ||
4. Run `grunt` command from terminal. It will generate `svg-min`, React (`esm` and `cjs`), `svg-sprite`, `pdf`, `php`, and `docs`. | ||
5. Commit | ||
6. Merge & delete branch | ||
|
||
|
@@ -81,19 +95,19 @@ Note that the icons in this set are tied to be used in [Calypso](https://github. | |
|
||
This icon set uses a few automation scripts to ease the generation of new icons in a reliable way. In short, we require `node` and `grunt`. For detailed instructions check [the installation page](https://github.com/Automattic/gridicons/wiki/Installation). | ||
|
||
Once you checkout the repo run `npm install` in the `gridicons` folder. | ||
To generate all the fonts, svgs and so on you run `npm run build` | ||
Once you checkout the repo run `npm install` in the `gridicons` folder. | ||
To generate all the fonts, svgs and so on you run `npm run build` | ||
|
||
## Publishing to npm | ||
|
||
Note: to proceed with this you need to have write authorization to npm. | ||
|
||
1. Create a new PR with updated `CHANGELOG.md` and updated version in `package.json` (i.e. `1.2.3-alpha.1`), see an example [here](https://github.com/Automattic/gridicons/pull/275). | ||
2. In the "CHANGELOG.md" make sure to check all the previous commits since the previous versioned release. | ||
1. Create a new PR with updated `CHANGELOG.md` and updated version in `package.json` (i.e. `1.2.3-alpha.1`), see an example [here](https://github.com/Automattic/gridicons/pull/275). | ||
2. In the "CHANGELOG.md" make sure to check all the previous commits since the previous versioned release. | ||
3. Pre-publish that PR branch on npm with `npm publish --tag next` ([more info](https://docs.npmjs.com/cli/dist-tag)). Running the `npm publish --tag next` command will send the data that you have localy to npm. Having the alpha version in the `package.json` file will create a newly tagged version npm package. Use `npm view gridicons` to look at the list of current tags. You do not need to commit changes to github in order to publish to npm, but it is recommended so folks testing know what's available. | ||
4. Create a new update PR in a repository that makes use of Gridicons and run `npm install gridicons@next --save` (which will update `packages.json`). If you're creating the PR in [Calypso](https://github.com/Automattic/wp-calypso) and you get warnings, it might need to regenerate the shrinkwrap, in which case run `npm run update-deps`. See an example [here](https://github.com/Automattic/wp-calypso/pull/17601). | ||
4. Create a new update PR in a repository that makes use of Gridicons and run `npm install gridicons@next --save` (which will update `packages.json`). If you're creating the PR in [Calypso](https://github.com/Automattic/wp-calypso) and you get warnings, it might need to regenerate the shrinkwrap, in which case run `npm run update-deps`. See an example [here](https://github.com/Automattic/wp-calypso/pull/17601). | ||
5. Test if the new icons show up, and there are no regressions in the previous icons. Take a look at the `http://calypso.localhost:3000/devdocs/design/gridicons` for example. | ||
6. If changes look good, remove the alpha postfix in the version (i.e. `1.2.3-alpha.1` to `1.2.3`) on both repositories PRs. | ||
6. If changes look good, remove the alpha postfix in the version (i.e. `1.2.3-alpha.1` to `1.2.3`) on both repositories PRs. | ||
7. Merge the Gridicons PR. | ||
8. Tag the release on GitHub: `git tag -a v1.2.3 -m "Release v1.2.3"` (and push `git push origin v1.2.3`). | ||
9. Check if it shows up in the [Releases list](https://github.com/Automattic/gridicons/releases). | ||
|
@@ -106,11 +120,11 @@ Gridicons is licensed under [GNU General Public License v2 (or later)](./LICENSE | |
|
||
## More notes on publishing to npm | ||
You need to have a npm user account. [Create one here](https://www.npmjs.com/signup). | ||
Once you have created it, set up the account on you machine via | ||
Once you have created it, set up the account on you machine via | ||
$ `npm adduser` | ||
|
||
Setup the 2fa with npm | ||
$ `npm profile enable-2fa` | ||
Setup the 2fa with npm | ||
$ `npm profile enable-2fa` | ||
|
||
Now everytime before you can publish | ||
Now everytime before you can publish | ||
You will be asked for a your [2FA code (OPT)](https://en.wikipedia.org/wiki/One-time_password) |
This file was deleted.
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,87 @@ | ||
const iconsThatNeedOffset = [ | ||
'gridicons-add-outline', | ||
'gridicons-add', | ||
'gridicons-align-image-center', | ||
'gridicons-align-image-left', | ||
'gridicons-align-image-none', | ||
'gridicons-align-image-right', | ||
'gridicons-attachment', | ||
'gridicons-bold', | ||
'gridicons-bookmark-outline', | ||
'gridicons-bookmark', | ||
'gridicons-calendar', | ||
'gridicons-cart', | ||
'gridicons-create', | ||
'gridicons-custom-post-type', | ||
'gridicons-external', | ||
'gridicons-folder', | ||
'gridicons-heading', | ||
'gridicons-help-outline', | ||
'gridicons-help', | ||
'gridicons-history', | ||
'gridicons-info-outline', | ||
'gridicons-info', | ||
'gridicons-italic', | ||
'gridicons-layout-blocks', | ||
'gridicons-link-break', | ||
'gridicons-link', | ||
'gridicons-list-checkmark', | ||
'gridicons-list-ordered', | ||
'gridicons-list-unordered', | ||
'gridicons-menus', | ||
'gridicons-minus', | ||
'gridicons-my-sites', | ||
'gridicons-notice-outline', | ||
'gridicons-notice', | ||
'gridicons-plus-small', | ||
'gridicons-plus', | ||
'gridicons-popout', | ||
'gridicons-posts', | ||
'gridicons-scheduled', | ||
'gridicons-share-ios', | ||
'gridicons-star-outline', | ||
'gridicons-star', | ||
'gridicons-stats', | ||
'gridicons-status', | ||
'gridicons-thumbs-up', | ||
'gridicons-textcolor', | ||
'gridicons-time', | ||
'gridicons-trophy', | ||
'gridicons-user-circle', | ||
'gridicons-reader-follow', | ||
'gridicons-reader-following' | ||
]; | ||
const iconsThatNeedOffsetY = [ | ||
'gridicons-align-center', | ||
'gridicons-align-justify', | ||
'gridicons-align-left', | ||
'gridicons-align-right', | ||
'gridicons-arrow-left', | ||
'gridicons-arrow-right', | ||
'gridicons-house', | ||
'gridicons-indent-left', | ||
'gridicons-indent-right', | ||
'gridicons-minus-small', | ||
'gridicons-print', | ||
'gridicons-sign-out', | ||
'gridicons-stats-alt', | ||
'gridicons-trash', | ||
'gridicons-underline', | ||
'gridicons-video-camera' | ||
]; | ||
const iconsThatNeedOffsetX = [ | ||
'gridicons-arrow-down', | ||
'gridicons-arrow-up', | ||
'gridicons-comment', | ||
'gridicons-clear-formatting', | ||
'gridicons-flag', | ||
'gridicons-menu', | ||
'gridicons-reader', | ||
'gridicons-strikethrough' | ||
]; | ||
|
||
module.exports = { | ||
iconsThatNeedOffset, | ||
iconsThatNeedOffsetX, | ||
iconsThatNeedOffsetY, | ||
}; |
This file was deleted.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we avoid acronyms (cjs, esm) in favor of a clear, spelled out folder name?
common-js
ecmascript
Also: can we add somewhere a detail on why there's this split, and as a developer when should I pick one or the other?
In genereal: is there any way to avoid this split for a simpler solution that reduces decision fatigue?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a very good question I myself pondered about.
We can have a default
import GridiconExternal from 'gridicons/external'
that pulls the CommonJS module (which is now the common format used in npm packages). That was actually the original proposal, but it would compromise future backward compatibility, as it was raised by @ockham Some notes on this:cjs
andesm
. There is no a strong standard on how to do it yet. I wish we could just avoid this module war, but we can't.cjs
icons undergridicons
, whenesm
becomes more used we won't be able to use that namespace and maintain backward compatibility. We'll need to publish the individual icons undergridicons/esm
, for example. This would put us in a weird position and could potentially introduce errors if the consumer is expecting aesm
module when usinggridicons/external
, for example.esm
option is deemphasized, which was intentional to mitigate decision fatigue.gridicons
an alias forgridicons/cjs
orgridicons/esm
- this is probably what we'll do in Calypso.Does this address your questions @folletto ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, this is totally on me @folletto 😄
@nosolosw summed it up excellently. I'll just add that having untranspiled code available in
esm/
allows the consuming project to make use of features such as tree-shaking. I.e. with some further iterations, we'll be able to do handy named imports such aswhile still ensuring that only the code that's relevant for these imports is being pulled in, thanks to tree-shaking -- i.e. we can get both nice named imports, and smaller bundle sizes. This is something we can't do with CommonJS.
(For reference, my original suggestion was at #283 (comment))
The different (CJS vs ESM) entrypoints are exposed through the
main
andesnext
fields inpackage.json
; the general idea is taken from http://2ality.com/2017/06/pkg-esnext.html (and as Andrés said, we can use e.g. webpack aliases to still allow 'pretty' import directory filenames, e.g. without the explicitcjs/
oresm/
parts).I've also seen some (albeit not a lot) of standardization towards these names (
cjs/
oresm/
), so I'd rather keep them. I'd like to think of them as an implementation detail that typically is aliased by the consumer anyway.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the reply. The technical answer is grounding for the question I have. I'm concerned on how cryptic now this is. Before "react/build" was clear. Now there's choice, with two three-letter shorthands that require knowing all of what you described to make a choice.
Granted, it's going to be packaged and published on npm, but is there any way to make this more understandable also in this library?
Also: I'm not sure if this is the case but when I read "it would compromise future backward compatibility" are we talking about Calypso or...? If Calypso already works with the more modern format, I'd suggest to ship just that and bump the requirements if needed. Or, at least, create a strong default for that, and leave the other as a secondary option.
At the very very least: can we pick a default, and have a one line explanation on when to use each of these two? Imagine a developer coming in and having no idea on the difference between the two when you write these one liners.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
'Default' now depends on context: If you project is CommonJS/ES5 only, use
cjs/
. If it supports ES6 stuff -- most notably modules -- either natively, or thru transpilation, useesm/
. This is somwhat exposed by the relevant fields inpackage.json
, but I guess we can also document in theREADME
. Sounds good?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@folletto @ockham wasn't aware of your comments when I posted this. Does it modifies your thinking @folletto or you still hold on this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ockham Good point, that was a mistake on my side. Both CJS and ES subpackages require moving files to package root. I edited my comment accordingly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@nosolosw WFM!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In case there is a top-level
index.js
that reexports submodules:If we import only
GridiconNotice
, how does Webpack and UglifyJS know they can omit theGridiconSearch
import? They don't, because importing the./search
module can have side effects. For example, it can change some global variable or register a DOM listener.The
sideEffect: false
flag says that no such side effects exist and unused imports can be safely removed.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@nosolosw Exporting just the CJS modules and forgetting about ES6 now works for me. The syntax would be:
In future, when Webpack 4 becomes ubiquitous, we can migrate to full ES6:
In the meantime, we could have a Babel transform that lets us write the named import syntax right now and transform it to a
gridicons/dist/${icon}
import. We have something like this for Lodash and Redux Form already in Calypso. But that's optional.I hope I summarized everything correctly :)