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

Use the whole contents to load the correct font #923

Closed
dvoytenko opened this issue Apr 1, 2020 · 3 comments · Fixed by #1275
Closed

Use the whole contents to load the correct font #923

dvoytenko opened this issue Apr 1, 2020 · 3 comments · Fixed by #1275
Assignees
@obetomuniz
Labels
Group: Fonts P0 Critical, drop everything Type: Bug Something isn't working

Comments

@dvoytenko
Copy link
Contributor

dvoytenko commented Apr 1, 2020 •
edited by jauyong

Bug Description

(Blocked by #921)

The different font styles are not loaded in the output document. A similar problem could also be present in the editor as well.

  1. Enter a text element
  2. Set "Roboto" font
  3. Customize different parts of it using side panel and text selection as well. Use italics and bold.
  4. Save and preview.

Let's assume that the resulting content was:

some text
<strong>some bold</strong>
<i>some italic</strong>
愛 means "love" in Japanese

The resulting document is observed to have:

<link rel="stylesheet" id="edit-story_fonts-css" href="https://fonts.googleapis.com/css?family=Roboto%3A400%7C&amp;subset=latin%2Clatin-ext&amp;display=swap&amp;ver=1.0.0-alpha" media="all">

Expected:

  • "Roboto:ital" for italic styles/contents
  • "Roboto:...wght" for 700 (bold), etc.
  • Possibly even a more expanded character subset to capture Japanese.

Do not alter or remove anything below. The following sections will be managed by moderators only.

Acceptance Criteria

QA Instructions
@dvoytenko dvoytenko added the Type: Bug Something isn't working label Apr 1, 2020
@dvoytenko dvoytenko mentioned this issue Apr 1, 2020
Closed
@spacedmonkey
Copy link
Contributor

So it is worth noting that, the when Roboto in the editor, it loads all the font variants. This may seem waste and it is, but was done to make sure that if the user changes the weight or style of the text, the font is already in the browser, Waiting for a new font to load in while a font is set to bold is jarring.

I do agree through that in the FE ( story that is presented to the user ), we should not load all the fonts. I think we should only load correct styles and font weights. We can also limit the font by the characters used to give an even smaller font file to the user. But that benefit of this maybe limited by the download be slower, is the query string is not cached in CDN etc.

@dvoytenko dvoytenko mentioned this issue Apr 6, 2020
Closed
@jauyong jauyong added the P0 Critical, drop everything label Apr 7, 2020
@pbakaus pbakaus added this to the 1.0 Alpha milestone Apr 11, 2020
@swissspidy swissspidy modified the milestones: 1.0 Alpha, 1.0 Beta Apr 14, 2020
@kmyram kmyram modified the milestones: 1.0 Beta, Sprint 27 Apr 16, 2020
@ndev1991
Copy link
Contributor

@kmyram This seems blocked by #921, but it's not on the list of next sprint?

@swissspidy swissspidy mentioned this issue Apr 20, 2020
Merged
3 tasks
@miina miina mentioned this issue May 6, 2020
Merged
37 tasks
@miina miina self-assigned this May 6, 2020
@miina miina mentioned this issue May 6, 2020
Merged
obetomuniz added a commit that referenced this issue May 7, 2020
@obetomuniz
Adjustments after #1323 merge. Revert content as parameter to load fo…
b7a18ba 
…nt (To address #923).
@miina
Copy link
Contributor

miina commented May 8, 2020

@obetomuniz Will assign this to you since most likely you'll cover it in #1275
Feel free to unassign yourself if it comes out that the PR won't be addressing it afterall.

@miina miina assigned obetomuniz and unassigned miina May 8, 2020
barklund pushed a commit that referenced this issue May 19, 2020
@obetomuniz @swissspidy
Add resize support to box when changing font-face on display/edit mode (
bc47950 
#1275)

* Add resize support to box when changing font-face on display/edit mode

* Cover unicode chars, font-weight, font-style

* Add ((MULTIPLE)) support proposal

* Improve docs.

* Fix problematic rest approach.

* Add fontSize support. Remove content support. Add a better support for MULTIPLE. Improve useLoadFontFiles.js

* Update textStyle tests

* Force display=auto on @font-face declaration while loading fonts from Google Fonts.

* Fix typo

* Add codecov to useLoadFontFiles

* Update tests to match new APIs proposed after merge.

* Address PR reviews

* Address some PR reviews.

* Adjustments after #1323 merge. Revert content as parameter to load font (To address #923).

* Address recent PR review

* Improve code performance/reusability. Thanks @dvoytenko

* Clean up promise logic a bit

Co-authored-by: Pascal Birchler <pascalb@google.com>
Co-authored-by: Morten Barklund <morten.barklund@xwp.co>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@obetomuniz obetomuniz

Labels
Group: Fonts P0 Critical, drop everything Type: Bug Something isn't working
Projects
None yet
Milestone
No milestone
9 participants
@pbakaus @spacedmonkey @dvoytenko @swissspidy @kmyram @obetomuniz @miina @jauyong @ndev1991