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

Update openhtmltopdf. Add visual regression test script #166

Merged
merged 2 commits into from
Nov 24, 2023

Conversation

jamesbarnett91
Copy link
Contributor

@jamesbarnett91 jamesbarnett91 commented Nov 23, 2023

Update openhtmltopdf. We were on quite an old version so there is risk of regression issues.
However, since we now have an API, I've written a quick script to walk the API and generate all labels, and ran it against both this updated branch and the existing QA deployment (which still has the old version of openhtmltopdf). Then ran a pixel based diff of both images to check for any visual regression between the labels generated with the old and new versions of openhtmltopdf.

I've attached the diff images as a zip diff-images.zip (or you can run the script yourself). The image is a composite of diff, old version then new version, left to right. Only labels with any change have been included, so any not in the zip have not changed.

There are some interesting differences but they are all very minor, mostly related to text anti-aliasing. In some cases the new version seems more 'correct', such as the pluses on Range Hoods, or the radiator icon on Solid Fuel Boilers.

As part of approving this PR could you check the image diffs and confirm you're happy?

Copy link
Contributor

@matteason matteason left a comment

Choose a reason for hiding this comment

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

I'm happy with the diffs that only affect anti-aliasing and fix the radiators

The only real regression I can see is the font on the kg icon on tumble dryers has changed, I think from Verdana to Arial

Before:
image

After:
image

But looking at the template and legislation I think it actually should have been Myriad all along anyway:
image

I didn't spot this before because it's so close to Verdana, but worth fixing now we've seen it. I'll send you the files for Myriad

@jamesbarnett91 jamesbarnett91 merged commit 729f726 into develop Nov 24, 2023
3 checks passed
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