Skip to content

lettering Icon Addition#5525

Merged
MichaelMarcialis merged 5 commits intoelastic:mainfrom
MichaelMarcialis:lettering-icon
Jan 14, 2022
Merged

lettering Icon Addition#5525
MichaelMarcialis merged 5 commits intoelastic:mainfrom
MichaelMarcialis:lettering-icon

Conversation

@MichaelMarcialis
Copy link
Contributor

@MichaelMarcialis MichaelMarcialis commented Jan 10, 2022

Summary

Added a new lettering icon to EUI. Closes #4825.

image

Checklist

  • Check against all themes for compatibility in both light and dark modes
  • Checked in Chrome, Safari, Edge, and Firefox
  • Added documentation
  • Added or updated jest and cypress tests
  • A changelog entry exists and is marked appropriately

@cchaos cchaos requested a review from elizabetdev January 10, 2022 20:16
@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_5525/

Copy link
Contributor

@elizabetdev elizabetdev left a comment

Choose a reason for hiding this comment

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

Thanks, @MichaelMarcialis for opening this PR.

The code looks great. 🎉

But I have a suggestion. When compared with the other icons, the lettering icon seems thinner. I noticed that it has 1px of weight like the other icons, but it's perceived as thinner.

What do you think of changing the font weight to medium?

lettering-preview@2x

@MichaelMarcialis
Copy link
Contributor Author

When compared with the other icons, the lettering icon seems thinner. I noticed that it has 1px of weight like the other icons, but it's perceived as thinner.

What do you think of changing the font weight to medium?

Thanks for the comment, @miukimiu! While it looks similar to Inter regular, I actually ended up drawing this lettering glyph by hand to ensure that it aligned better to the pixel grid. If you don't feel like it's important to align to the grid though, let me know and I can swap it out for a version that uses Inter medium.

Copy link
Contributor

@elizabetdev elizabetdev left a comment

Choose a reason for hiding this comment

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

Thanks @MichaelMarcialis,

I run some tests using the inter medium. I had to do some adjustments to make it align better to the pixel grid. But even though, when I did some browser testings in low res it didn't look so good.

So let's stick to your version which is perfectly aligned to the pixel grid! 🎉

@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_5525/

@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_5525/

@MichaelMarcialis MichaelMarcialis merged commit 2810211 into elastic:main Jan 14, 2022
@MichaelMarcialis MichaelMarcialis deleted the lettering-icon branch January 14, 2022 19:33
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.

Initials icon

3 participants