Skip to content

[UI] Remove Font Awesome #14265

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 12 commits into from
Jan 10, 2025
Merged

[UI] Remove Font Awesome #14265

merged 12 commits into from
Jan 10, 2025

Conversation

andersonjeccel
Copy link
Contributor

Q A
Bug fix? (use the a.b branch) 🔴
New feature/enhancement? (use the a.x branch) 🔴
Deprecations? 🟢
BC breaks? (use the c.x branch) 🟢
Automated tests included? 🔴
Related user documentation PR URL mautic/user-documentation#...
Related developer documentation PR URL mautic/developer-documentation-new#...
Issue(s) addressed Fixes #...

Description

This PR removes Font Awesome.

@andersonjeccel andersonjeccel changed the title Remove Font Awesome [UI] Remove Font Awesome Nov 13, 2024
@andersonjeccel andersonjeccel self-assigned this Nov 13, 2024
@andersonjeccel andersonjeccel added user-interface Anything related to appearance, layout, and interactivity code-review-needed PR's that require a code review before merging bc-break A BC break PR for major release milestones only deprecation Includes deprecations labels Nov 13, 2024
@andersonjeccel andersonjeccel added this to the 6.0 milestone Nov 13, 2024
escopecz
escopecz approved these changes Nov 14, 2024
View reviewed changes
Copy link
Member

@escopecz escopecz left a comment

Choose a reason for hiding this comment

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

Nice slimming here! I read through the changes and it all makes sense. I clicked through the UI around custom fields, dashboard and campaign and didn't notice any issues. Although they would be hard to spot in case of a missing icon. I also compared the 5.x branch and this in terms of the performance and there is a slight improvement.
Screenshot 2024-11-14 at 11 46 06
I'm wondering whether we need all the fonts loading?

@escopecz escopecz added pending-test-confirmation PR's that require one test before they can be merged code-review-passed PRs which have passed code review and removed code-review-needed PR's that require a code review before merging labels Nov 14, 2024
@andersonjeccel
Copy link
Contributor Author

@escopecz I’ve been trying to understand where are these fonts coming from for months, maybe the GrapesJS builder loads them even when not in use

It’d be great to make them local, because they’re loaded directly from Google Fonts (we’re never sure how they’re tracking everyone)

@andersonjeccel
Copy link
Contributor Author

@escopecz btw, there are thousands of CSS lines related to Froala (I heard it’s going to be removed in 6.x)

should I create a PR to remove these styles?

@escopecz
Copy link
Member

Yes, let's remove Froala for M6.

@RCheesley RCheesley linked an issue Nov 19, 2024 that may be closed by this pull request
[Deprecation] [6.0] Completely remove Font Awesome #14195
Closed
matbcvo
matbcvo approved these changes Dec 3, 2024
View reviewed changes
Copy link
Contributor

@matbcvo matbcvo left a comment

Choose a reason for hiding this comment

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

The PR has been tested and looks good to me, but there are merge conflicts.

@matbcvo matbcvo added has-conflicts Pull requests that cannot be merged until conflicts have been resolved user-testing-passed PRs which have been successfully tested by the required number of people. and removed pending-test-confirmation PR's that require one test before they can be merged labels Dec 4, 2024
@andersonjeccel andersonjeccel marked this pull request as draft December 5, 2024 12:56
@andersonjeccel andersonjeccel added the WIP PR's that are not ready for review and are currently in progress label Dec 5, 2024
@andersonjeccel
Copy link
Contributor Author

found hardcoded stuff in less files, I'll have to address them

@andersonjeccel andersonjeccel force-pushed the bc-remove-font-awesome branch from 55e22f2 to ef24d45 Compare January 9, 2025 19:15
@andersonjeccel andersonjeccel marked this pull request as ready for review January 9, 2025 19:16
@andersonjeccel andersonjeccel removed has-conflicts Pull requests that cannot be merged until conflicts have been resolved WIP PR's that are not ready for review and are currently in progress labels Jan 9, 2025
@codecov Codecov
Copy link

codecov bot commented Jan 10, 2025 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 63.67%. Comparing base (016b104) to head (ef24d45).
Report is 13 commits behind head on 6.x.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff            @@
##                6.x   #14265   +/-   ##
=========================================
  Coverage     63.67%   63.67%           
  Complexity    34625    34625           
=========================================
  Files          2276     2276           
  Lines        103576   103576           
=========================================
+ Hits          65949    65950    +1     
+ Misses        37627    37626    -1
Files with missing lines Coverage Δ
app/bundles/AssetBundle/Entity/Asset.php 73.76% <ø> (ø)
app/bundles/CoreBundle/Model/NotificationModel.php 56.52% <ø> (ø)
...les/EmailBundle/EventListener/ReportSubscriber.php 64.30% <ø> (ø)
...uticClearbitBundle/Controller/PublicController.php 0.00% <ø> (ø)
...cFullContactBundle/Controller/PublicController.php 0.00% <ø> (ø)

... and 1 file with indirect coverage changes

@escopecz escopecz merged commit 0d57852 into mautic:6.x Jan 10, 2025
17 checks passed
@andersonjeccel andersonjeccel deleted the bc-remove-font-awesome branch January 10, 2025 10:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@matbcvo matbcvo matbcvo approved these changes

@escopecz escopecz escopecz approved these changes

Assignees

@andersonjeccel andersonjeccel

Labels
bc-break A BC break PR for major release milestones only code-review-passed PRs which have passed code review deprecation Includes deprecations user-interface Anything related to appearance, layout, and interactivity user-testing-passed PRs which have been successfully tested by the required number of people.
Projects
UX/UI Tiger Team
Status: Done
Milestone
6.0.0-alpha
Development

Successfully merging this pull request may close these issues.

[Deprecation] [6.0] Completely remove Font Awesome
3 participants
@andersonjeccel @escopecz @matbcvo