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 prawn-icon to version 4.1.0 #2550

Open
wants to merge 1 commit into
base: v2.3.x
Choose a base branch
from

Conversation

pepijnve
Copy link
Member

This PR updates prawn-icon to v4.1.0

The main change in 4.1.0 is the upgrade of Font Awesome to v6. An extra feature has been added that allows to keep the icon size consistent even though the font metrics of FAv6 are a bit different to those of v5.

There are still two failing visual comparison tests. I wasn't sure if I could just update the reference. FWIW, the computer sees a difference, my eyes don't.

@mojavelinux
Copy link
Member

There are still two failing visual comparison tests. I wasn't sure if I could just update the reference. FWIW, the computer sees a difference, my eyes don't.

That's typical of these visual comparison tests. Sometimes it's a rounding difference that has no perceivable visual impact. What I typically do is check to see if it looks right, then just commit the updated reference file.

@mojavelinux
Copy link
Member

I needed to make sure that upgrading to FontAwesome6 was not a breaking change. Here's what the docs say:

Upgrading should be easy. But Downgrading could be difficult. Version 6 has some new icons, styles, and styling goodies. Once you’ve started using them in your project, downgrading to Version 5 may result in missing icons and styling.

So it appears that this upgrade is not a breaking change and thus should not require a major release. We're cheating a bit by putting it in a patch release, but we don't plan to make more minor releases of this major version line, so I'm willing to accept it.

@mojavelinux
Copy link
Member

Hmm, I now realize that we've been intentionally holding prawn-icon back in the v2.3.x branch for reasons of compatibility. The main branch used 3.1.0 while the v2.3.x branch uses 3.0.0. I really wonder whether this change should be made in the main branch instead. That might mean we need to accelerate the release of Asciidoctor PDF 3.0.0.

See #2507

@mojavelinux
Copy link
Member

Sometimes it's a rounding difference that has no perceivable visual impact.

I see what it is. Some of the icons have slightly different weights after the upgrade.

After seeing that, I'm convinced this change needs to target Asciidoctor PDF 3.0.0 (the main branch).

I'm also a little concerned about the two assertion changes regarding the font size. I understand that the font size has to be adjusted to achieve the icon height...but I'd like to verify that visually that means the output is identical as before the upgrade.

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