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

Change font family, fix invalid CSS (fixes #70) #253

Merged
merged 2 commits into from
Oct 1, 2019
Merged

Change font family, fix invalid CSS (fixes #70) #253

merged 2 commits into from
Oct 1, 2019

Conversation

niutech
Copy link
Contributor

@niutech niutech commented Feb 1, 2019

CSS values image-rendering: optimizeSpeed and optimize-contrast are invalid, as are // single-line comments.
Now with font-family: 'Courier New', monospace a web page text fits its background image (issue #70).

@tobimensch
Copy link
Collaborator

@niutech
Thanks for your pull request.

Through the linked issue it appears you're using "Windows 10 with devicePixelRatio=1.25".

Did you test your patch with any other system, i.e. Linux?

@niutech
Copy link
Contributor Author

niutech commented Feb 6, 2019

No, right now I have only Windows 10 machine, I will test it as soon as I get to the Linux computer.
Meanwhile I have edited the PR not to change the font-size, but to use Courier New font if available, because it fixes the issue with font width (default monospace on Windows 10 is Consolas, which is narrower).

@niutech niutech changed the title Increase font size, fix invalid CSS (fixes #70) Change font family, fix invalid CSS (fixes #70) Feb 6, 2019
@tobimensch
Copy link
Collaborator

@niutech
You probably know this, however I just wanted to mention that you could also test this in a Linux virtual machine on Windows.

@niutech
Copy link
Contributor Author

niutech commented Feb 6, 2019 via email

@niutech
Copy link
Contributor Author

niutech commented Feb 11, 2019

OK, I have tested the changes under Lubuntu Linux. Here is Chromium with default monospace font:
chromium-monospace

Here is Chromium with 'Courier New' font:
chromium-courier-new

And here is Firefox with the new font:
firefox-courier-new

The new font does not break page layout nor change font size on Linux, but it fixes the layout on Windows 10, so I propose to merge this fix.

As a side note, I think the default width (number of columns) on html.brow.sh is too narrow for today's screen sizes. I would increase it to 140.

Thanks!

@tombh
Copy link
Member

tombh commented Feb 14, 2019

Thanks so much for this, for doing the research, finding a fix and submitting the screenshots. I'm happy to merge this. Just one thing, but don't worry if you don't get round to it, could you squash these 2 commits and just create a little commit message giving the context to these changes.

@niutech
Copy link
Contributor Author

niutech commented Mar 26, 2019

You can easily squash commits by yourself while merging: https://github.blog/2016-04-01-squash-your-commits/

@tombh tombh force-pushed the master branch 4 times, most recently from f290601 to 81f41b7 Compare June 19, 2019 05:12
@niutech
Copy link
Contributor Author

niutech commented Sep 16, 2019

@tombh Will you merge this PR?

@tombh tombh merged commit f62d7dc into browsh-org:master Oct 1, 2019
@tombh
Copy link
Member

tombh commented Oct 1, 2019

Sorry for the delay. By asking you to rebase I was hoping you'd be able to better name and describe the commit. Then I just forgot about it.

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

3 participants