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

Add line height config option #2858

Merged
merged 1 commit into from
Apr 20, 2018
Merged

Add line height config option #2858

merged 1 commit into from
Apr 20, 2018

Conversation

bdougherty
Copy link
Contributor

@bdougherty bdougherty commented Apr 17, 2018

Add a lineHeight configuration parameter. Not sure if what I did is the appropriate way to check that the type is correct, so please let me know if there is a different way that it's normally done and I will update it.

This will fix #75 and fix #2850.

@Andy-set-studio Andy-set-studio mentioned this pull request Apr 18, 2018
2 tasks
@timothyis timothyis added this to the 2.1.0 milestone Apr 18, 2018
@timothyis timothyis requested a review from chabou April 18, 2018 15:46
@chabou
Copy link
Collaborator

chabou commented Apr 19, 2018

Awesome! Thank you! I'll merge it ASAP

Copy link
Contributor

@albinekb albinekb left a comment

Choose a reason for hiding this comment

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

Looks great to me 👍

const fontSizeChanged = this.props.fontSize !== nextProps.fontSize;
const fontFamilyChanged = this.props.fontFamily !== nextProps.fontFamily;
const lineHeightChanged = this.props.lineHeight !== nextProps.lineHeight;
if (fontSizeChanged || fontFamilyChanged || lineHeightChanged) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think that storing comparison result in temp vars is not necessary.
These comparisons don't need some explanation with explicit variable names. And without these temporary variables, comparisons will be done only if previous one is invalid.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated.

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.

Awesome thank you

@chabou
Copy link
Collaborator

chabou commented Apr 20, 2018

I tried it and it works great.

The only problem is maybe cursor size. For example with a double line height:
Double line-height

But not a big deal (and on xterm part). It could be the desired behavior.

@chabou chabou merged commit bba14f6 into vercel:canary Apr 20, 2018
@bdougherty bdougherty deleted the line-height branch April 20, 2018 22:34
@Andy-set-studio
Copy link

I've built the canary build and this is working really well. Thanks for the hard work @bdougherty!

@dongz9
Copy link

dongz9 commented Apr 23, 2018

@chabou I prefer to keep the cursor size unchanged. Do you think it's reasonable to make it configurable?

@jesseleite
Copy link

Love the cursor size, how lines look like with background highlighting, etc. Great implementation! I can stop using my line height patched font now 😬Thank you! ❤️

@baevra
Copy link

baevra commented Apr 25, 2018

Line height is awesome!!!
But now we need cursor height option!

@jesseleite
Copy link

Yall crazy, love the tall cursor. Wouldn't a shorter cursor look funny next to background highlighted text?

@albinekb
Copy link
Contributor

we could default cursorHeight to be the same as lineHeight @jesseleite 👍

@Azzhoe
Copy link

Azzhoe commented Apr 27, 2018

So has this been fixed yet, I still don't see an option to adjust line height anywhere in the latest 2.0.0 Stable build. Everything is so close together by default, and it looks terrible and makes things more difficult to read.

@albinekb
Copy link
Contributor

No, it's not in stable yet @Azzhoe https://github.com/zeit/hyper/releases/tag/2.1.0-canary.1

@rchrdnsh
Copy link

rchrdnsh commented Oct 5, 2018

Any update on this? When will 2.1.0 be stable?

@Tuxie
Copy link

Tuxie commented Nov 27, 2018

I tried canary.2 but I was not able to set a lineHeight below 1.0. Is this a bug or a technical limitation? I want to reduce line height because my favourite font has too much spacing. In iTerm2 I have set line height to 85% and it looks perfect.

chabou pushed a commit that referenced this pull request Dec 15, 2018
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.

Configure line height Add line-height configuration parameter