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

fix(svgicon): set inline-block #673

Merged
merged 1 commit into from
Oct 19, 2021
Merged

fix(svgicon): set inline-block #673

merged 1 commit into from
Oct 19, 2021

Conversation

anniehu4
Copy link
Contributor

@anniehu4 anniehu4 commented Oct 18, 2021

Summary:

Related to https://github.com/FB-PLP/traject/pull/8898 -- the old EDSCandidates icon set flex-none, while the new EDS icon only set display: inline. In trying to unravel why, I noticed that:

I think we always wanted @apply flex-none inline-block and something got lost in the splitting 🤔

  • docs for display: inline state that "Any height and width properties will have no effect", which doesn't sound right for svg icon. In contrast, inline-block does respect height/width
  • flex: none "sizes the flex item according to the width / height of the content, but doesn't allow it to shrink", which we do want dubious if we want -- consumer can set flex themselves

these also match the Material UI icon styles

Test Plan:

no visual changes per Percy

@anniehu4 anniehu4 requested a review from a team October 18, 2021 17:51
Copy link
Contributor

@jinlee93 jinlee93 left a comment

Choose a reason for hiding this comment

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

Thanks again for the fix~~

Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

size-limit report

Path Size
components 56.05 KB (0%)
styles 3.91 KB (0%)

@@ -1,5 +1,5 @@
.svgIcon {
@apply inline;
@apply flex-none inline-block;
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it odd for the icon to set it's own flex properties?

I wonder if we should leave that up to users of the component. For example, if the LP left nav applies display: flex, should it be the one to also set any flex properties on the items?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

mm fair point. I thought it might have been standard b/c material was doing it but seems they're in the minority actually

@anniehu4 anniehu4 changed the title fix(svgicon): set flex-none fix(svgicon): set inline-block Oct 18, 2021
@codecov
Copy link

codecov bot commented Oct 18, 2021

Codecov Report

Merging #673 (88eb429) into main (f1f92e2) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##             main     #673   +/-   ##
=======================================
  Coverage   90.62%   90.62%           
=======================================
  Files          23       23           
  Lines         384      384           
  Branches       96       96           
=======================================
  Hits          348      348           
  Misses         36       36           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f1f92e2...88eb429. Read the comment docs.

Copy link
Contributor

@dierat dierat left a comment

Choose a reason for hiding this comment

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

I don't remember exactly, but what I'm guessing happened is that I didn't see a clear purpose for that CSS and removed it (thinking it might have been a mistake or no longer relevant). Leaving the flex property to the parent makes sense to me; I don't think I would expect that to come on the atomic component (if I were a dev using it in my feature).

@anniehu4 anniehu4 merged commit fe9f5fc into main Oct 19, 2021
@anniehu4 anniehu4 deleted the ahu/svgicon-flex branch October 19, 2021 21:51
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