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

Changed inline-blocks to flex for better centering #122

Closed
wants to merge 0 commits into from

Conversation

XenorPLxx
Copy link

@XenorPLxx XenorPLxx commented Apr 24, 2019

I've changed some display properties to flex, since centering was hard with inline-block. Height was always too high for me comparing to the component used. In the example below component is size 16 feather icon Star component from https://github.com/feathericons/react-feather. Before my changes span was height was 24px.

Btw.: There's many changes due to rebuilding and running yarn install. Should I remove those from PR?

image

@@ -35,13 +35,14 @@ class RatingSymbol extends React.PureComponent {
display: 'inline-block',
position: 'absolute',
overflow: 'hidden',
lineHeight: 0,
Copy link
Owner

Choose a reason for hiding this comment

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

Is this really necessary?

Copy link
Author

Choose a reason for hiding this comment

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

After some thought lineHeigth: unset might also work and definitely will be cleaner solution.
I'll check that and will get back to you in the evening.
It was necessary in my case: component I've pasted above can be used as inline-block next to some text that has high line-height and if span in react-rating inherits that line-height, then border around react-rating component becomes taller as well, as it has to stretch to accommodate for react-rating's height.
I'll post some screen in the evening along with results of my testing of unset param.

@dreyescat
Copy link
Owner

dreyescat commented Apr 25, 2019

I see... Yes, just changing inline-block to flex removes this extra height.

The only thing I would like you to confirm is if the lineHeight: 0 rule is completely necessary or we could just live with the flex display change.

Btw.: There's many changes due to rebuilding and running yarn install. Should I remove those from PR?

Yes please. Exclude changes under the lib folder. They are files that are auto-generated on release.

Thanks!

@XenorPLxx
Copy link
Author

XenorPLxx commented Apr 25, 2019

Okay, I know why I've done this. Here's example of what happens when there's diplay: flex on span around fullSymbol. I wasn't able to make that use display: flex and not stretch to width: 50% at the same time.

lineHeight: unset didn't work.

Screenshot 2019-04-25 at 11 48 12

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.

None yet

2 participants