Skip to content

Line icons addition#5633

Merged
MichaelMarcialis merged 5 commits intoelastic:mainfrom
MichaelMarcialis:line-icons
Feb 15, 2022
Merged

Line icons addition#5633
MichaelMarcialis merged 5 commits intoelastic:mainfrom
MichaelMarcialis:line-icons

Conversation

@MichaelMarcialis
Copy link
Copy Markdown
Contributor

@MichaelMarcialis MichaelMarcialis commented Feb 11, 2022 •
edited
Loading

Summary

Added new lineDashed, lineDotted, and lineSolid icons to EUI.

image

CCing @mbondyra, as elastic/kibana#124970 mentions needing these icons as part of the proposed design updates to Lens flyouts.

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

@MichaelMarcialis MichaelMarcialis changed the title Line icons Line icons addition Feb 11, 2022
@kibanamachine
Copy link
Copy Markdown

kibanamachine commented Feb 11, 2022

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

@cchaos cchaos requested a review from elizabetdev February 14, 2022 18:27
elizabetdev
elizabetdev suggested changes Feb 14, 2022
View reviewed changes
Copy link
Copy Markdown
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 adding this new great set of icons to EUI! 🎉

The icons look good to me. But I noticed that the lineDotted looks thinner when compared with the other two.

Screenshot 2022-02-14 at 19 25 41

What do you think of instead of making a circle of 2px x 2px making it instead with 3px x 3px. This change makes the circles bigger with more space between them, and it's easier to notice that they are dots.

Screenshot 2022-02-14 at 19 25 35

I guess one of the reasons you went with the 2px x 2px circles version is that they get better centered aligned. The 3px x 3px doesn't get centered properly but when is side by side with the other icons it doesn't make that much difference.

Screenshot 2022-02-14 at 19 29 22

Let me know what do you think of the proposed changes?

@MichaelMarcialis
Copy link
Copy Markdown
Contributor Author

MichaelMarcialis commented Feb 15, 2022

What do you think of instead of making a circle of 2px x 2px making it instead with 3px x 3px.

Thanks for the feedback, @miukimiu! I've updated the dottedLine icon to use 3x3px dots. Updated screenshot in the original description above.

@kibanamachine
Copy link
Copy Markdown

kibanamachine commented Feb 15, 2022

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

elizabetdev
elizabetdev approved these changes Feb 15, 2022
View reviewed changes
Copy link
Copy Markdown
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.

Tested in Safari, Firefox, and Chrome. Also tested in low resolution and LGTM! 🎉

@MichaelMarcialis MichaelMarcialis merged commit 5c552ad into elastic:main Feb 15, 2022
@MichaelMarcialis MichaelMarcialis deleted the line-icons branch February 15, 2022 19:26
@cee-chen cee-chen mentioned this pull request Feb 16, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@elizabetdev elizabetdev elizabetdev approved these changes

Reviewers whose approvals may not affect merge requirements

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

@MichaelMarcialis @kibanamachine @elizabetdev