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

Fix rendering in Firefox on Windows #5038

Merged
merged 10 commits into from
May 5, 2020
Merged

Fix rendering in Firefox on Windows #5038

merged 10 commits into from
May 5, 2020

Conversation

ftoh
Copy link
Contributor

@ftoh ftoh commented May 3, 2020

Fix #4813
Complements #4979
May be fixed #4275

Should be tested on other OS

Documentation for text-rendering attribute:
https://developer.mozilla.org/ru/docs/Web/SVG/Attribute/text-rendering

Comparison
image

image

@shields-ci
Copy link

shields-ci commented May 3, 2020

Messages
📖 ✨ Thanks for your contribution to Shields, @ftoh!

Generated by 🚫 dangerJS against 7d121a1

@calebcartwright calebcartwright added the npm-package Badge generation and badge templates label May 3, 2020
@paulmelnikow paulmelnikow changed the base branch from jamaisvu to master May 4, 2020 14:30
@paulmelnikow
Copy link
Member

This is amazing @ftoh! Thanks so much for tracking this down!

I re-added DejaVu Sans at the end. Would you mind updating the snapshots again?

Otherwise I think this is good to merge.

@calebcartwright does this look good to you on all the platforms? @RedSparr0w @chris48s any concerns?

@shields-cd shields-cd temporarily deployed to shields-staging-pr-5038 May 4, 2020 15:44 Inactive
@chris48s
Copy link
Member

chris48s commented May 4, 2020

Looks like there are still some failing tests - probably because the font order has changed.

The easiest thing to do is usually just to delete the snapshots file, re-run the package tests and then check the diff, rather than try to manually update the snapshots.

Beyond that, I think the core change here is useful

@paulmelnikow paulmelnikow temporarily deployed to shields-staging-pr-5038 May 4, 2020 22:13 Inactive
@calebcartwright
Copy link
Member

calebcartwright commented May 4, 2020

What's the page/listing shown in the screenshots so that i can compare from ubuntu?

@paulmelnikow - not sure if this is what you were asking for but screenshots from Firefox on Ubuntu below

image

image

@paulmelnikow
Copy link
Member

They look great, yea? Thanks!

@paulmelnikow paulmelnikow merged commit 6ca2fa0 into badges:master May 5, 2020
@calebcartwright
Copy link
Member

The changes in those screenshots on Windows look great.

I'm not sure if I was supposed to be able to see any differences on Linux. I felt a bit like this 😆
https://youtu.be/gW2LtX1217s?t=124

@paulmelnikow
Copy link
Member

Thanks for this awesome contribution to Shields @ftoh! I so appreciate your digging into this, and helping us make good on our "pixel perfect" promise.

If you're interested in continuing to dig in on these kinds of issues there's lots to do!

These are two really impactful examples:

Recently @chris48s and I rewrote all the badge generation using plain JavaScript and template literals, so it's much more readable and changeable than when they were DoT templates. (See #4459 and #3637. There is a lot of refactoring that can be done to make it even more readable.)

IIRC there are a handful of open issues relating to padding computations that are slightly off. With the code as it is now it should be pretty reasonable to track down the issues and fix them.

@ftoh
Copy link
Contributor Author

ftoh commented May 5, 2020

@paulmelnikow When will the changes take effect?

@paulmelnikow
Copy link
Member

It's live now!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
npm-package Badge generation and badge templates
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Improve text appearance in Firefox on Windows Letter spacing is incorrect on social badges
6 participants