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

don't calculate pixel width of input #694

Merged
merged 1 commit into from
Oct 28, 2019
Merged

don't calculate pixel width of input #694

merged 1 commit into from
Oct 28, 2019

Conversation

tinovyatkin
Copy link
Contributor

@tinovyatkin tinovyatkin commented Oct 25, 2019

I'm probably missing something, as there is no visual test for this functionality, but.
You currently have a great deal of code to calculate exact pixel width of helper INPUT element and it's doing good job. However, I think that almost the same functionality with little bit better compatibility with accessibility displays and other rare cases, as well as with much less code can be obtained by using well supported ch unit.
ch is a width of 0 in current font and it's not exact measurements of value string width, but you are using pre and magic number of 75% in this function anyway, so this variant can be tuned in similar way (something like length * 0.75 should be pixel perfect) if you want. For my taste, I will leave it like this.

Also found that current tests are using non-existent _placeholderValue property, but still passing well 🧐

Unfortunately, this functionality can't be tested with JSDOM due to lack of ch support there (I've filled appropriate Pull Request), so, I've tested by temporary using em units, and also manually in browser.

Now, it's up to you, as it's not pixel perfect and have some edge case where it leaves a little of space at the end on some strings. But I ❤️ this solution.

@tinovyatkin tinovyatkin changed the title don't calculate pixel width don't calculate pixel width of input Oct 25, 2019
@tinovyatkin
Copy link
Contributor Author

My Pull Request to JSDOM/cssstyle was merged tonight, so, soon it will be also testeable feature (with next JSDOM release hopefully).

@jshjohnson
Copy link
Collaborator

This is awesome 👌 thanks

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