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

Preload material icons, allow to preload other fonts #427

Merged
merged 4 commits into from
Jul 31, 2023
Merged

Preload material icons, allow to preload other fonts #427

merged 4 commits into from
Jul 31, 2023

Conversation

Oksydan
Copy link
Contributor

@Oksydan Oksydan commented Jan 8, 2023

Questions Answers
Description? CLS in header caused by material icons can be reduced by preloading material icons font. Unfortunately that icon font with version > 6.X got some weight. It is 125 KB for .woff2 format which is rly high. Comparing it to version that is classic theme using it's twice as heavy. I was thinking about preloading font via query param added to font-face src but it is impossible via webpack-font-preload-plugin plugin since it is including files query params with extension. It causing to fail at isFont condition. We can think about forking it or create our own plugin.
Type? improvement
BC breaks? no
Deprecations? no
Fixed ticket? Fixes #252 - partially
How to test? Build theme and check waterfall in network tab and loading of fonts in FO.
Possible impacts? no

Comparison - throttling 3G fast:

Before:

Nagranie.z.ekranu.2023-01-8.o.01.01.39.mov

After:

Nagranie.z.ekranu.2023-01-8.o.01.01.59.mov

NeOMakinG
NeOMakinG previously approved these changes Jan 8, 2023
View reviewed changes
Copy link
Contributor

@NeOMakinG NeOMakinG left a comment

Choose a reason for hiding this comment

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

That's cool we needed it, forking is most of the time increasing the maintenance cost more than anything

Anyway, even my comment is nitpicking so it's a 🍬 for me!

templates/_partials/preload.tpl Outdated Show resolved Hide resolved
0x346e3730
0x346e3730 previously approved these changes Jan 9, 2023
View reviewed changes
@Oksydan Oksydan dismissed stale reviews from 0x346e3730 and NeOMakinG via 1069a49 January 9, 2023 09:40
@Oksydan Oksydan requested a review from NeOMakinG January 9, 2023 09:40
NeOMakinG
NeOMakinG previously approved these changes Jan 9, 2023
View reviewed changes
kpodemski
kpodemski requested changes Mar 31, 2023
View reviewed changes
Copy link
Contributor

@kpodemski kpodemski left a comment

Choose a reason for hiding this comment

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

It's been a while... I wanted to approve it and merge it, but there's a package-lock conflict.

@Oksydan Oksydan force-pushed the preload-material-icons-font branch from 1069a49 to 7806eb8 Compare April 3, 2023 22:21
@Oksydan
Copy link
Contributor Author

Oksydan commented Apr 3, 2023

@kpodemski branch rebased.
Looks like some dependencies were missing in pacakge-lock.json 🤔

@Oksydan Oksydan requested a review from kpodemski April 5, 2023 20:25
kpodemski
kpodemski requested changes May 8, 2023
View reviewed changes
Copy link
Contributor

@kpodemski kpodemski left a comment

Choose a reason for hiding this comment

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

I wanted to accept it again, but you added Swiper.js. Not sure what's the reason. I don't see the usage of Swiper in Hummingbird.

@Oksydan
Copy link
Contributor Author

Oksydan commented May 8, 2023

@kpodemski fixed.
Looks like i did mistake during rebase process.
Swiper has been removed here 5a654af - it's been here when I was working with this feature.

@Oksydan Oksydan requested a review from kpodemski May 8, 2023 12:09
@kpodemski
Copy link
Contributor

@Hlavtox this has to be QA'd by a dev, do you think PS SA devs could help you with that?

@kpodemski
Copy link
Contributor

@ga-devfront @GytisZum guys, could you validate this PR? let me know if it looks ok for you

ga-devfront
ga-devfront reviewed Jun 23, 2023
View reviewed changes
Copy link
Collaborator

@ga-devfront ga-devfront left a comment

Choose a reason for hiding this comment

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

Thank's for your work, I just add one question about the configuration.

webpack/webpack.parts.js
}),
new FontPreloadPlugin({
index: 'preload.html',
extensions: ['woff2'],
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why only woff2 I know eot is only for IE and we don't support it but ttf is supported by all browser.

@ga-devfront ga-devfront reopened this Jul 27, 2023
@kpodemski kpodemski added this to the Beta milestone Jul 31, 2023
@kpodemski kpodemski merged commit 4fccbaa into PrestaShop:develop Jul 31, 2023
6 checks passed
@kpodemski
Copy link
Contributor

thanks @Oksydan ;-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ga-devfront ga-devfront ga-devfront left review comments

@kpodemski kpodemski kpodemski approved these changes

@0x346e3730 0x346e3730 0x346e3730 left review comments

@NeOMakinG NeOMakinG NeOMakinG left review comments

Assignees
No one assigned
Labels
Waiting for dev Waiting for QA
Projects
None yet
Milestone
Beta
Development

Successfully merging this pull request may close these issues.

Optimize bundles sizes, add csso, fix lighthouse issues
5 participants
@Oksydan @kpodemski @NeOMakinG @0x346e3730 @ga-devfront