Skip to content

Mdi icons instead of fontastic icons #192

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 4 commits into from
Dec 30, 2017
Merged

Mdi icons instead of fontastic icons #192

merged 4 commits into from
Dec 30, 2017

Conversation

VeraZab
Copy link
Contributor

@VeraZab VeraZab commented Dec 29, 2017 •
edited
Loading

here's how it looks like:
screen shot 2017-12-29 at 3 42 20 pm

screen shot 2017-12-29 at 3 41 51 pm

for reference: https://github.com/plotly/plotly-icons

@VeraZab
Copy link
Contributor Author

VeraZab commented Dec 29, 2017

ugh... I'm not sure I understand these errors @bpostlethwaite
screen shot 2017-12-29 at 5 00 44 pm

@bpostlethwaite
Copy link
Member

We expect node_modules files to be es5. But you are importing something from node_modules that is es6 and Nodejs doesn't understand the syntax.

@bpostlethwaite
Copy link
Member

We'll Nodejs expects valid JS for whatever version of Nodejs is running Jest. Anyway you need to transport the icon code if you use es6. Otherwise you should write in es5 which most browsers and Nodejs versions support

@VeraZab
Copy link
Contributor Author

VeraZab commented Dec 30, 2017

any objections on merging this one @bpostlethwaite @aulneau ?

@bpostlethwaite
Copy link
Member

ok reviewing now

@aulneau
Copy link
Contributor

aulneau commented Dec 30, 2017

I can't review it right now but it looks visually good to me!

bpostlethwaite
bpostlethwaite previously approved these changes Dec 30, 2017
View reviewed changes
Copy link
Member

@bpostlethwaite bpostlethwaite left a comment

Choose a reason for hiding this comment

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

One super tiny nit but this is great stuff! 💃

src/components/containers/MenuPanel.js Outdated
return {
iconClass: 'menupanel__icon icon-question-circle',
iconType: <QuestionIcon className="menupanel__icon" />,
Copy link
Member

Choose a reason for hiding this comment

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

This could just be named icon as iconType makes me think that it will be a string describing the type of icon.

src/styles/_shame.scss
line-height: 1;
-webkit-font-smoothing: antialiased;
-moz-osx-font-smoothing: grayscale;
}
Copy link
Member

Choose a reason for hiding this comment

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

👏

src/styles/icons/_icons.scss
}
.icon-link:before {
content: '\67';
}
Copy link
Member

Choose a reason for hiding this comment

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

👏

src/styles/main.scss
display: flex;
flex-grow: 1;
}
}

@import 'shame';
Copy link
Member

Choose a reason for hiding this comment

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

🎉

@VeraZab
Copy link
Contributor Author

VeraZab commented Dec 30, 2017

I'll merge for now @aulneau but if anything, can fix it in a subsequent pr

@VeraZab VeraZab merged commit 8854925 into master Dec 30, 2017
@VeraZab VeraZab deleted the mdi-icons branch December 30, 2017 19:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bpostlethwaite bpostlethwaite bpostlethwaite left review comments

@aulneau aulneau Awaiting requested review from aulneau

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@VeraZab @bpostlethwaite @aulneau