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

Use Vazir font with a node module instead of CDN #27611

Merged
merged 20 commits into from
Apr 6, 2022

Conversation

NeOMakinG
Copy link

@NeOMakinG NeOMakinG commented Feb 7, 2022

Questions Answers
Branch? develop
Description? Using a CDN exposes users to a risk of data abuse and privacy abuse, so we swap to a local font using npm
Type? improvement
Category? BO
BC breaks? no
Deprecations? no
Fixed ticket? Fixes #27541.
How to test? RTL language font should be loaded normally (tested it, works fine), also when you run npm run build on default, new-theme and classic/_dev folder, it should generate a thirdPartyNotice.json file in the asset folder, containing every notices
Possible impacts? RTL font

This change is Reviewable

@prestonBot prestonBot added develop Branch Improvement Type: Improvement labels Feb 7, 2022
@NeOMakinG NeOMakinG marked this pull request as ready for review February 7, 2022 10:40
@NeOMakinG NeOMakinG requested a review from a team as a code owner February 7, 2022 10:40
@NeOMakinG
Copy link
Author

Can be QA by a dev I think so!

matks
matks previously approved these changes Feb 8, 2022
Copy link
Contributor

@matks matks left a comment

Choose a reason for hiding this comment

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

I remember we discussed using a CDN, yes or no, for another topic.

Maybe we should add a mention in devdocs FAQ about this?

@NeOMakinG
Copy link
Author

I remember we discussed using a CDN, yes or no, for another topic.

Maybe we should add a mention in devdocs FAQ about this?

could probably be useful yes

@matthieu-rolland matthieu-rolland added the Waiting for dev Status: action required, waiting for tech feedback label Feb 9, 2022
@mparvazi
Copy link
Member

mparvazi commented Feb 27, 2022

Great improvement 💯

Some suggestions:

1- Just need to use woff and woff2 for font-face (to me woff2 is enough!)
Why you should be responsible for users who are stuck in the past? (eot only uses by IE, ttf only needed for very old browsers). IMO a browser that doesn't support woff2, also it doesn't support some components in BO.

@font-face {
  font-family: Vazir;
  font-style: normal;
  font-display: swap;
  font-weight: 300;
  src: url("~vazir-font/dist/Vazir-Light.woff2") format("woff2"), url("~vazir-font/dist/Vazir-Light.woff") format("woff");
}

@font-face {
  font-family: Vazir;
  font-style: normal;
  font-display: swap;
  font-weight: 400;
  src: url("~vazir-font/dist/Vazir-Regular.woff2") format("woff2"), url("~vazir-font/dist/Vazir-Regular.woff") format("woff");
}

@font-face {
  font-family: Vazir;
  font-style: normal;
  font-display: swap;
  font-weight: 700;
  src: url("~vazir-font/dist/Vazir-Bold.woff2") format("woff2"), url("~vazir-font/dist/Vazir-Bold.woff") format("woff");
}

The number of fonts will be reduced from 15 to 6 in the public folder.

2- The rtl_rtl.css will be generated by CSSJanus in public folder, but only rtl.css will be used, so need to prevent transforming rtl.css by CSSJanus (PHP plugin that use by StylesheetGenerator.php). Because in future you may put overrides of RTL inside rtl.scss (plan to stop using theme.rtlfix).
src/Core/Localization/RTL/StylesheetGenerator.php
proccess

3- We don't have any call for rtl.css in Login page:
controllers/admin/AdminLoginController.php
Screenshot from 2022-02-27 11-41-24

4- The license text for vazir-font is null. I think it's related to missing "LICENSE" file in project's root folder.
Screenshot from 2022-02-26 16-21-05
Can use replenishDefaultLicenseTexts option (when this is enabled, default license texts are taken from spdx.org for packages where no license text was found):
Screenshot from 2022-02-27 11-43-34

5- After webpack build for default theme, there is an empty rtl.bundle.js file, so we can use webpack-remove-empty-scripts.

@NeOMakinG
Copy link
Author

@mparvazi Every points are fixed, except the rtl.css with CSSJanus, actually we can't do it, it's not a problem for the moment but we need to take care about the fact that no one add something else than fonts inside rtl.scss

@NeOMakinG NeOMakinG removed the Waiting for dev Status: action required, waiting for tech feedback label Feb 28, 2022
PierreRambaud
PierreRambaud previously approved these changes Feb 28, 2022
@mparvazi
Copy link
Member

@NeOMakinG
I tested and works fine.

matks
matks previously approved these changes Mar 1, 2022
@matks matks added the Documentation ✔️ Developer documentation is up-to-date label Mar 1, 2022
@matks matks added this to the 8.0.0 milestone Mar 1, 2022
@matks matks added the Waiting for QA Status: action required, waiting for test feedback label Mar 1, 2022
@matks
Copy link
Contributor

matks commented Mar 1, 2022

@Robin-Fischer-PS @NeOMakinG I suggest a dev does the QA?

@NeOMakinG
Copy link
Author

@matks yeah, why not!

@Robin-Fischer-PS
Copy link
Contributor

@matks @NeOMakinG I need some more info : Is there a high risk of regressions on RTL display ? If so, I would like to have a regression test made by one of the QA girls.

@NeOMakinG
Copy link
Author

@matks @NeOMakinG I need some more info: Is there a high risk of regressions on RTL display? If so, I would like to have a regression test made by one of the QA girls.

  • RTL won't work fine anymore on IE11 (which is not supported anymore in june) and some very old browsers with low usage
  • No high risk of regressions on other browsers

Progi1984
Progi1984 previously approved these changes Mar 22, 2022
Co-authored-by: Progi1984 <progi1984@gmail.com>
@matks matks added Waiting for QA Status: action required, waiting for test feedback Waiting for dev Status: action required, waiting for tech feedback Needs documentation Needs an update of the developer documentation and removed Documentation ✔️ Developer documentation is up-to-date labels Mar 24, 2022
@matks matks self-assigned this Apr 6, 2022
@matks matks added QA ✔️ Status: check done, code approved and removed Waiting for QA Status: action required, waiting for test feedback Waiting for dev Status: action required, waiting for tech feedback labels Apr 6, 2022
@matks
Copy link
Contributor

matks commented Apr 6, 2022

I checked out the branch and tested. As far as I see it works as expected.

@matks matks merged commit 948f3e4 into PrestaShop:develop Apr 6, 2022
@matks matks added the Key feature Notable feature to be highlighted label Apr 6, 2022
@kpodemski kpodemski added Documentation ✔️ Developer documentation is up-to-date Needs documentation Needs an update of the developer documentation and removed Needs documentation Needs an update of the developer documentation Documentation ✔️ Developer documentation is up-to-date labels Jul 28, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
develop Branch Documentation ✔️ Developer documentation is up-to-date Improvement Type: Improvement Key feature Notable feature to be highlighted QA ✔️ Status: check done, code approved
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Remove dependency of Fonts CDNs in the PrestaShop open source project
10 participants