-
-
Notifications
You must be signed in to change notification settings - Fork 32.5k
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
[docs] Use material-ui-icons package #6390
Conversation
@@ -35,6 +35,7 @@ | |||
}, | |||
"dependencies": { | |||
"marked": "^0.3.6", | |||
"material-ui-icons": "^1.0.0-alpha.2", |
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.
Why not changing the webpack conf to use the icons on the repository?
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.
I fear the maintenance cost of keeping up to date.
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.
@oliviertassinari - the icons aren't in the repository. I guess we could add them, but unless there's major change, there is no maintenance cost.
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.
@mbrookes Alright, choose what you think is best 👍 .
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.
This is simplest for now. We can always revisit it if it causes an unforseen problem.
A visual regression test fails |
@oliviertassinari I might need your help to get the visual regression tests working again on my machine. In the meantime, would you mind fixing whatever i broke for this PR? 😊 If you wanted to add the es6 icons to the repo at the same time, I'm fine with that on reflection, as it would mean we don't need to keep a separate copy of the icons used in components in |
@mbrookes I'm updating the baseline. The sooner argos-ci will be on, the better. We will simply be able to accept the changes. |
That sounds ideal! 👍 |
PR has
tests/ docs demo, and is linted.Commit and PR titles begin with [ComponentName], and are in imperative form: "[Component] Fix leaky abstraction".
Description explains the issue / use-case resolved, and auto-closes the related issue(s) (http://tr.im/vFqem).
Replace local icons with packaged icons
Remove unneeded icons from src/svg-icons
Replace font-icon with SvgIcons in
IconButton
sReplace uses of Icon with material-ui-icons (except
Icon
docs examples)Update & expand Style > Icons doc page