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

Material icon font #190

Merged
merged 6 commits into from
Jul 2, 2020
Merged

Material icon font #190

merged 6 commits into from
Jul 2, 2020

Conversation

lkogler
Copy link
Contributor

@lkogler lkogler commented Jun 18, 2020

Addresses issue #154

@hartsick hartsick temporarily deployed to honeycrisp-material-ico-9ltczu June 18, 2020 22:14 Inactive
@lkogler lkogler requested a review from bengolder June 18, 2020 22:16
Copy link
Contributor

@bengolder bengolder left a comment

Choose a reason for hiding this comment

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

It looks like this adds a lot more icons that we can use, so it would solve the need expressed in #154

@lkogler
Copy link
Contributor Author

lkogler commented Jun 18, 2020

Whoa, looks like percy caught a real issue with this change -- some of the icons nested in other components are broken! Will need to investigate and resolve before merging.

lkogler and others added 5 commits June 18, 2020 17:56
- PARTIALLY fixes issue #154
- should investigate ligatures as an option in the future

Co-authored-by: Symonne Singleton <[email protected]>
- Added `$icons` as variables in `_variables.rb`

Fixes issue #154

Co-authored-by: Symonne Singleton <[email protected]>
The `project-icons` map is aan optional parameter in the icons mixin.
This map, which can be added to a project's custom SCSS file, can be used to
add icons that do not currently exist in the Honeycrisp library.

Co-authored-by: Symonne Singleton <[email protected]>
* Information on how to use an icon if we don't have a predefined class for it
 - replace direct usages of character codepoint with variables
 - fix $font-icon variable value and use it everywhere we set the icon font-family
 - Adjust margin-top on .flash__message .flash--error because the warning icon was appearing too high with new icon set
 - Set visited color on flash message dismiss button so it doesn't change color once it has been "visited"
@lkogler lkogler force-pushed the material-icon-font branch from ada2093 to 90a5a97 Compare June 19, 2020 01:51
@lkogler lkogler temporarily deployed to honeycrisp-material-ico-9ltczu June 19, 2020 01:52 Inactive
@lkogler lkogler requested a review from racheledelman June 22, 2020 18:12
@lkogler
Copy link
Contributor Author

lkogler commented Jun 22, 2020

Can you take a look at the Percy diffs @racheledelman ? It looks like the material icon font generally renders a couple pixels higher up than the old font. I'm not sure how much you care about the difference. It could be time consuming to fix, but we can do that if you feel it's important.

@racheledelman
Copy link

Hm, I would like to fix that, sorry! We'll need to be able to align icons with texts.

@lkogler lkogler temporarily deployed to honeycrisp-material-ico-9ltczu July 1, 2020 00:21 Inactive
@lkogler lkogler merged commit 8b70d43 into master Jul 2, 2020
@lkogler lkogler deleted the material-icon-font branch July 2, 2020 20:07
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.

4 participants