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

[pulsar-next] Round line-height of text-editor v2 #1187

Conversation

asiloisad
Copy link
Contributor

@asiloisad
Copy link
Contributor Author

Looks like jackpot. I do not see any side effects, but I will wait until someone more experienced check it @savetheclocktower

@savetheclocktower
Copy link
Contributor

It looks great! I was worried about the possibility of the measured lineHeight getting out of sync with our rounded-off version, but I should've realized that the measured line height is downstream of the CSS changes we've made and will implicitly stay in sync with them. I jumped to the conclusion that the bug had something to do with the layout measurement code because I'd only recently touched it.

I'll play around with this a little bit more because I think we can safely do half-pixels as well, especially if devicePixelRatio is 2. I'm a little scared of what happens when devicePixelRatio is 3, but I can't find any examples of a devicePixelRatio that high in the wild except for a reference to a Samsung phone.

On my machine, half-pixel granularity makes it so that every increase or decrease of 0.1 to editor.lineHeight results in a difference in the editor, so I don't think the rounding will even be noticed or minded.

Well done!

@savetheclocktower
Copy link
Contributor

OK, I addressed this in 3d3c6bc. I applied the device-pixel technique and simplified the line height measurement in TextEditorComponent.

This seems to work quite well for me, but grab the latest from updated-latest-electron and tell me if it causes you problems.

Thanks again!

@asiloisad
Copy link
Contributor Author

works as desired. thank you!

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.

2 participants