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

TASK: Upgrade to FontAwesome 6.5.2 #3801

Merged
merged 9 commits into from
Jun 25, 2024

Conversation

wiegandj
Copy link
Collaborator

@wiegandj wiegandj commented Jun 1, 2024

Issue: #3793

Upgraded FontAwesome in neos-ui to 6.5.2.

Versions were tested in neos-development-collection before.

See upgrade for neos-development-collection:
Issue: neos/neos-development-collection#5107
PR: neos/neos-development-collection#5108

@mhsdesign mhsdesign changed the base branch from 8.3 to 8.4 June 2, 2024 20:03
@github-actions github-actions bot added 8.4 and removed 8.3 labels Jun 2, 2024
@ahaeslich ahaeslich requested a review from grebaldi June 3, 2024 15:44
@ahaeslich
Copy link
Member

@wiegandj can you please rebase this change onto the 8.4 branch. This way no unrelated commits will show up in your PR.

@Sebobo Sebobo requested review from Sebobo and ahaeslich June 5, 2024 10:11
Copy link
Member

@Sebobo Sebobo left a comment

Choose a reason for hiding this comment

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

I had to ignore some type related issues with our FA Icons component. We can either merge it like this and fix this separately or find a fix.
The TS error message was too convoluted for me to understand right now.

Ping @grebaldi @markusguenther @mhsdesign maybe one of you can understand the error.

@ahaeslich ahaeslich linked an issue Jun 5, 2024 that may be closed by this pull request
@grebaldi
Copy link
Contributor

@Sebobo

I had to ignore some type related issues with our FA Icons component. We can either merge it like this and fix this separately or find a fix.
The TS error message was too convoluted for me to understand right now.

Our IconProps extend the FontAwesomeIconProps imported from @fortawesome/react-fontawesome. Since FortAwesome/react-fontawesome@e32a0cb (so, v0.2.0 of @fortawesome/react-fontawesome) FontAwesomeIconProps extends RefAttributes<SVGSVGElement>, which implies the following definition for a ref prop:

{
  ref?: Ref<SVGSVGElement>
}

This definition collides with the intrinsic ref prop that is defined for every class component, in the case of our FontAwesomeIcon component that would be:

{
  ref?: LegacyRef<FontAwesomeIcon>
}

This can be easily fixed by adjusting the type definition for our IconProps (in

export interface IconProps extends Omit<FontAwesomeIconProps, 'icon'> {
) thusly:

-export interface IconProps extends Omit<FontAwesomeIconProps, 'icon'> {
+export interface IconProps extends Omit<FontAwesomeIconProps, 'icon' | 'ref'> {

If it's easier, I think it'd be okay to do this post-merge. But this branch needs a rebase anyway.

@Sebobo Sebobo force-pushed the upgrade-fontawesome-to-6-5-2 branch from b84066d to ce0a356 Compare June 25, 2024 06:29
mhsdesign and others added 7 commits June 25, 2024 08:29
Currently translated at 100.0% (121 of 121 strings)

Translation: Neos/Neos.Ui - Main - 8.3
Translate-URL: https://hosted.weblate.org/projects/neos/neos-ui-main-8-3/nl/
Currently translated at 100.0% (121 of 121 strings)

Translation: Neos/Neos.Ui - Main - 8.3
Translate-URL: https://hosted.weblate.org/projects/neos/neos-ui-main-8-3/es/
There are some issues with the updated TS types from FA 6 which don’t work with out code.
We ignore the issue for now, but could either find a fix in this PR or separate the problem.
With the change the UI builds fine and the new icons can be used.
@Sebobo Sebobo force-pushed the upgrade-fontawesome-to-6-5-2 branch from ce0a356 to dcd3808 Compare June 25, 2024 06:37
@Sebobo
Copy link
Member

Sebobo commented Jun 25, 2024

I rebased, solved conflicts and fixed the type issue like you described @grebaldi, thx a lot!
I also found that icons in dropdowns were suddenly to large as the font awesome class fa-w-16 does not exist anymore (so much for not breaking). So I had to fix that in the places I found.

Copy link
Contributor

@grebaldi grebaldi left a comment

Choose a reason for hiding this comment

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

Thanks for taking care of the update, @wiegandj! And thanks for rebasing and solving conflicts @Sebobo!

I updated the failing snapshot test and fixed the icon reference in that one E2E test case. Everything should be green when CI is done.

@grebaldi grebaldi merged commit 2daad68 into neos:8.4 Jun 25, 2024
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

FEATURE: upgrade FontAwesome to latest version (6.5.2)
5 participants