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.

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

[UI] Remove Font Awesome #14265

Open
wants to merge 8 commits into
base: 6.x
Choose a base branch
from
Open

[UI] Remove Font Awesome #14265

wants to merge 8 commits into from
+33 −6,564

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
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@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 pending-test-confirmation PR's that require one test before they can be merged user-interface Anything related to appearance, layout, and interactivity
Projects
UX/UI Tiger Team
Status: In Progress
Milestone
6.0
Development

Successfully merging this pull request may close these issues.

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