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

Set line-height same as font size #1841

Merged
merged 3 commits into from
Jun 16, 2017
Merged

Set line-height same as font size #1841

merged 3 commits into from
Jun 16, 2017

Conversation

mihaliak
Copy link
Contributor

@mihaliak mihaliak commented May 18, 2017

Set x-row line-height same as font-size to fix issues with some fonts on last line of terminal.

This PR should fix #628

UPDATE: removed whole css as chabou said line-height is same as font size as default

Copy link
Collaborator

@chabou chabou left a comment

Choose a reason for hiding this comment

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

Could better to just remove this line. By default, line-height has the same value than font-size.

I don't know why we have this line-height of 1.2em set.
We must be sure to not introduce some regressions

@mihaliak
Copy link
Contributor Author

@chabou just removed whole css block for x-row, this should be fine

@mihaliak mihaliak changed the title x-row line-height same as font-size Removed x-row css May 18, 2017
@ppot
Copy link
Contributor

ppot commented May 18, 2017

@chabou @rauchg This was added by @dotcypress for emoji!

@mojoaxel
Copy link
Contributor

mojoaxel commented Jun 5, 2017

@chabou Removing the whole line doesn't fix #628 for me.
Instead I use :

x-row {
    line-height: 1em;
}

@chabou
Copy link
Collaborator

chabou commented Jun 7, 2017

You're right, we need to specify 1em to have emoji vertical aligned.
@mihaliak can you update your PR with @mojoaxel recommendations ?

I'll merge it. If it leads to some emoji regression we'll find another way to correct this regression. #628 really hurts

set x-row to 1em
Copy link
Contributor

@mojoaxel mojoaxel left a comment

Choose a reason for hiding this comment

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

LGTM fixes #628

@albinekb
Copy link
Contributor

I think this is 👌 we can fix regressions.

@chabou chabou changed the title Removed x-row css Set line-height same as font size Jun 16, 2017
@chabou chabou merged commit 5ec7050 into vercel:master Jun 16, 2017
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.

5 participants