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

updates to fonts #8756

Merged
merged 12 commits into from
Dec 1, 2021
Merged

updates to fonts #8756

merged 12 commits into from
Dec 1, 2021

Conversation

indirectlylit
Copy link
Contributor

@indirectlylit indirectlylit commented Nov 19, 2021 •
edited
Loading

Summary

  • Upgrading the FontTools dependency
  • Simplify font-loading code for "modern" browsers
  • Ensure that there are no conflicting glyphs in generated subsets
  • Updated to latest Noto fonts. Adds many new scripts:
    • Adlam
    • Balinese
    • Bamum
    • Gondi
    • Kayah Li
    • Lisu
    • Medefaidrin
    • Meetei Mayek
    • Ol Chiki
    • Sora Sompeng
    • Sundanese
    • Tai Tham

References

Reviewer guidance

This update should be an overall improvement of font handling, but I haven't thoroughly checked impact on a range of browsers. If someone could help check the status of these issues in particular I'd appreciate it:

Testing checklist

  • Contributor has fully tested the PR manually
  • If there are any front-end changes, before/after screenshots are included
  • Critical user journeys are covered by Gherkin stories
  • Critical and brittle code paths are covered by unit tests

PR process

  • PR has the correct target branch and milestone
  • PR has 'needs review' or 'work-in-progress' label
  • If PR is ready for review, a reviewer has been added. (Don't use 'Assignees')
  • If this is an important user-facing change, PR or related issue has a 'changelog' label
  • If this includes an internal dependency change, a link to the diff is provided

Reviewer checklist

  • Automated test coverage is satisfactory
  • PR is fully functional
  • PR has been tested for accessibility regressions
  • External dependency files were updated if necessary (yarn and pip)
  • Documentation is updated
  • Contributor is in AUTHORS.md

@indirectlylit indirectlylit changed the title fonts udpates to fonts Nov 19, 2021
@indirectlylit indirectlylit added this to the 0.15.0 milestone Nov 19, 2021
@indirectlylit
Copy link
Contributor Author

@rtibbles I'll take a look at #7805 as a follow-up

@indirectlylit indirectlylit added the TODO: needs review Waiting for review label Nov 20, 2021
@radinamatic radinamatic changed the title udpates to fonts updates to fonts Nov 20, 2021
@indirectlylit indirectlylit marked this pull request as draft November 20, 2021 00:32
@indirectlylit
Copy link
Contributor Author

converting to draft – found an issue

@indirectlylit indirectlylit marked this pull request as ready for review November 22, 2021 20:32
@indirectlylit
Copy link
Contributor Author

ready for review

@radinamatic
Copy link
Member

radinamatic commented Nov 23, 2021 •
edited
Loading

Tested the EXE asset in IE11, Firefox & Chrome on Windows 7, and Edge, Firefox & Chrome on Windows 10; found only 2 obvious issues:

  1. IE11/Windows 7: labels for Urdu and Hindi are missing in the language selection modal ONLY when the UI is in English.
    And IE11 still seems to be loading all full non-Latin font families...

    ie11

  2. Chrome on Windows 10, font color in modal has been changed...? Firefox and Edge seem unaffected.

    Win10 (start) Running - Oracle VM VirtualBox_006

@indirectlylit indirectlylit mentioned this pull request Nov 23, 2021
Merged
9 tasks
@radinamatic
Copy link
Member

Hm, in this latest build I can confirm that non-Latin fonts are not loading by default on IE11, but there is still the issue of the Urdu and Hindi labels missing in the change language modal... 😕

ie11-2

@indirectlylit
Copy link
Contributor Author

thanks for testing! will investigate early next week

@indirectlylit
Copy link
Contributor Author

IE11/Windows 7: labels for Urdu and Hindi are missing in the language selection modal ONLY when the UI is in English.
And IE11 still seems to be loading all full non-Latin font families...

This should be addressed in c5c2400

Chrome on Windows 10, font color in modal has been changed...? Firefox and Edge seem unaffected.

I'm not reproducing this in Browserstack. Any additional guidance?

modal browser details
image image

rtibbles
rtibbles reviewed Dec 1, 2021
View reviewed changes
Copy link
Member

@rtibbles rtibbles left a comment

Choose a reason for hiding this comment

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

No concerns from the code side - once this passes manual QA we're good to merge.

kolibri/core/assets/src/utils/setupAndLoadFonts.js
addFontStylesheetLink(plugin_data.fullCSSFileModern);
loadFullFontsImmediately();
Copy link
Member

Choose a reason for hiding this comment

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

Could return early here and avoid the else clause.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes! sounds like a comment I might usually leave :)

@radinamatic
Copy link
Member

With the latest build I'm not able to replicate past issues any more! 👏🏽

Labels for Urdu and Hindi in the language selection ✔️ Font color in modal on Chrome ✔️
Win7 (start) Running - Oracle VM VirtualBox_011 I retested in 2 completely different VMs just to be sure

Win10 (start) Running - Oracle VM VirtualBox_007

radinamatic
radinamatic approved these changes Dec 1, 2021
View reviewed changes
Copy link
Member

@radinamatic radinamatic left a comment

Choose a reason for hiding this comment

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

:shipit: 💯

@rtibbles rtibbles merged commit 36939a9 into learningequality:release-v0.15.x Dec 1, 2021
@indirectlylit indirectlylit deleted the fonts branch December 2, 2021 14:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rtibbles rtibbles rtibbles left review comments

@radinamatic radinamatic radinamatic approved these changes

Assignees
No one assigned
Labels
TODO: needs review Waiting for review
Projects
None yet
Milestone
0.15.0
Development

Successfully merging this pull request may close these issues.

inline noto-subset font load failed in Firefox
3 participants
@indirectlylit @radinamatic @rtibbles