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

Italics and Hide Font Variants #670

Closed
jauyong opened this issue Mar 20, 2020 · 27 comments · Fixed by #927
Closed

Italics and Hide Font Variants #670

jauyong opened this issue Mar 20, 2020 · 27 comments · Fixed by #927
Assignees

Comments

@jauyong
Copy link

jauyong commented Mar 20, 2020

Feature Description

During Grooming on March 18, there was discussion around having a separate issue to handle italics and hiding font variants.


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

Acceptance Criteria

  1. The Font weight dropdown should to be dynamic
    1. This should not require an API change. Font weights are exposed in the REST API & thus available in FontProvider
  2. Optimize enqueueing Google fonts so only the required fonts weights and variants are actually loaded
  3. We should handle enqueueing Google Fonts in the client-side generated AMP output for this (port \Google\Web_Stories\Story_Post_Type::load_fonts)

Note:

  1. Not all fonts support font weights. For example, Abel only supports "regular". No bold. No italic. How should we display the Italic toggle and the font weights dropdown in that case? cc @samitron7
  2. If we need to disable the Italic toggle, this would need an API change, see Expose available variants in fonts API endpoint #876 for that.)

QA Instructions

@dvoytenko
Copy link
Contributor

I believe the issue here is not removing font variants field at all. But instead we can remove "Italic" versions from the dropdown and automatically using "Italic" variants for italized markup.

@miina
Copy link
Contributor

miina commented Mar 26, 2020

Yep, as Dima said the issue was not about removing the font variants field.

Note that "Italic" is already not displayed among font variants.

An additional complication discussed for using italic variants was selecting just part of the text and marking it italic.

Related #413

@jauyong
Copy link
Author

jauyong commented Mar 26, 2020

Thank you. AC updated

@ndev1991 ndev1991 self-assigned this Mar 27, 2020
@ndev1991
Copy link
Contributor

ndev1991 commented Mar 30, 2020

Screen Shot 2020-03-30 at 6 04 49 AM

@swissspidy @miina @dvoytenko As we can see from the design, we don't have variants dropdown anymore. There is only font family and font weight dropdown select. Font variants like the Bold, Italic, Underline will be handled from Toggle selections.

@miina
Copy link
Contributor

miina commented Mar 30, 2020

@ndev1991 That's correct, the discussion was exactly about using the font variants in case of the toggle buttons -- to use the selected font variants (if exists) instead of the browser default italic and bold.

@ndev1991
Copy link
Contributor

@pbakaus @swissspidy @jauyong This issue for version 1.0, do we supposed to handle bold and italic for entire text only? (not partial selection of text)

@ndev1991
Copy link
Contributor

@spacedmonkey @miina /web-stories/v1/fonts I think this api is customized one. Where can I find this api document and function?

@spacedmonkey
Copy link
Contributor

@ndev1991 The code for the api can be found here.

For more details on the API try /web-stories/v1/, where you will be able to see all params.

Adding italic in, feels like it will need PHP work. So will have be me, Minna or Pascal that looks in this ticket, I think.

@ndev1991
Copy link
Contributor

@ndev1991 The code for the api can be found here.

For more details on the API try /web-stories/v1/, where you will be able to see all params.

Adding italic in, feels like it will need PHP work. So will have be me, Minna or Pascal that looks in this ticket, I think.

I see. @spacedmonkey @jauyong Can we have another ticket for api update here to unblock this issue?

@spacedmonkey
Copy link
Contributor

@ndev1991 It would be useful if you should define exactly what you need from WP and the REST API, before we create that ticket.

@ndev1991
Copy link
Contributor

@ndev1991 It would be useful if you should define exactly what you need from WP and the REST API, before we create that ticket.

@spacedmonkey Here is my understanding for this ticket. We are having font weights for each font when we get them through api. And that will be used for weights dropdown on font style panel.

Currently there are multiple toggle options for italic, underline and bold for text. For bold option I think we need to collaborate with weights dropdown so when selected as Bold from dropdown it should be selected on toggle and also same goes on toggle to dropdown. And for italic and underline we are using default browser function here. I think underline makes sense to use default browser section based on this ticket. But the italic should be handled based on that font option. And we don't have that kind of records on fonts from the api. So I think we will need to get variants option from web-stories/v1/fonts api to get variants for that font. I'm not sure what options can it be but italic should be required.

Would you confirm this is correct direction? @pbakaus @swissspidy @miina

@pbakaus
Copy link
Contributor

pbakaus commented Mar 30, 2020

@ndev1991 It would be useful if you should define exactly what you need from WP and the REST API, before we create that ticket.

@spacedmonkey Here is my understanding for this ticket. We are having font weights for each font when we get them through api. And that will be used for weights dropdown on font style panel.

Currently there are multiple toggle options for italic, underline and bold for text. For bold option I think we need to collaborate with weights dropdown so when selected as Bold from dropdown it should be selected on toggle and also same goes on toggle to dropdown. And for italic and underline we are using default browser function here. I think underline makes sense to use default browser section based on this ticket. But the italic should be handled based on that font option. And we don't have that kind of records on fonts from the api. So I think we will need to get variants option from web-stories/v1/fonts api to get variants for that font. I'm not sure what options can it be but italic should be required.

Would you confirm this is correct direction? @pbakaus @swissspidy @miina

Nope:

  1. With italic, we should only default to the browser italic when there's no italic front variant for the current font (see Roboto as example, has italic variants for all). To be clear, we hide the italic variants in the dropdown, and if the user clicks the "i" button, we switch to that variant under the hood, if it exists, and if not we use the browser native.
  2. Underline variants don't usually exist, so should always use the browser default here.
  3. Bold variants do exist, but apps like Google Slides always show them in the dropdown. But you're suggestion is good, which means that if you click "b", and there's a "Bold" variant (not Extra Bold, not Medium, just Bold), then we switch to that automatically in the dropdown. So to be clear, that means that if I select "Bold" in the dropdown, "b" is automatically selected as well (so works in both directions).
  4. An interesting edge case is "Extra Bold". We could either a), treat it like an unknown variant, and not select the "b" when the user selects the font in the dropdown, or b) treat it is a "Bold" variant, and select the "b" (and when you then click "b" again, it switches back to normal weight. I'm leaning towards b).

Let me know if that makes sense, happy to answer additional questions.

@ndev1991
Copy link
Contributor

@ndev1991 It would be useful if you should define exactly what you need from WP and the REST API, before we create that ticket.

@spacedmonkey Here is my understanding for this ticket. We are having font weights for each font when we get them through api. And that will be used for weights dropdown on font style panel.
Currently there are multiple toggle options for italic, underline and bold for text. For bold option I think we need to collaborate with weights dropdown so when selected as Bold from dropdown it should be selected on toggle and also same goes on toggle to dropdown. And for italic and underline we are using default browser function here. I think underline makes sense to use default browser section based on this ticket. But the italic should be handled based on that font option. And we don't have that kind of records on fonts from the api. So I think we will need to get variants option from web-stories/v1/fonts api to get variants for that font. I'm not sure what options can it be but italic should be required.
Would you confirm this is correct direction? @pbakaus @swissspidy @miina

Nope:

  1. With italic, we should only default to the browser italic when there's no italic front variant for the current font (see Roboto as example, has italic variants for all). To be clear, we hide the italic variants in the dropdown, and if the user clicks the "i" button, we switch to that variant under the hood, if it exists, and if not we use the browser native.
  2. Underline variants don't usually exist, so should always use the browser default here.
  3. Bold variants do exist, but apps like Google Slides always show them in the dropdown. But you're suggestion is good, which means that if you click "b", and there's a "Bold" variant (not Extra Bold, not Medium, just Bold), then we switch to that automatically in the dropdown. So to be clear, that means that if I select "Bold" in the dropdown, "b" is automatically selected as well (so works in both directions).
  4. An interesting edge case is "Extra Bold". We could either a), treat it like an unknown variant, and not select the "b" when the user selects the font in the dropdown, or b) treat it is a "Bold" variant, and select the "b" (and when you then click "b" again, it switches back to normal weight. I'm leaning towards b).

Let me know if that makes sense, happy to answer additional questions.

@spacedmonkey @jauyong @kmyram Can we have this api ticket created and be done before tomorrow morning? Unless the api ready until that time, I think this ticket can not be completed before Wednesday morning.

@jauyong
Copy link
Author

jauyong commented Mar 30, 2020

@ndev1991 I created #876 for the API update. Can you fill out exactly what you need from WP and the REST API in the ticket? And can you assign it to whoever would work on this? I'm not sure who it would be? @spacedmonkey

@ndev1991
Copy link
Contributor

@ndev1991 I created #876 for the API update. Can you fill out exactly what you need from WP and the REST API in the ticket? And can you assign it to whoever would work on this? I'm not sure who it would be? @spacedmonkey

@jauyong details added to issue #876 but not assigned to anyone yet. I think it php side so should be @spacedmonkey or @miina

@ndev1991
Copy link
Contributor

ndev1991 commented Mar 31, 2020

@spacedmonkey @swissspidy @miina @pbakaus So for this issue, we need to use the font variant like the italic if exists. And no way to detect if the font variants exists or not currently. I can't see any fields that related with font variants from the fonts api. So I request api update for font variants. So basically I think a new field required that shows the available variants for this fonts. Like variants: ['Italic']

@pbakaus
Copy link
Contributor

pbakaus commented Mar 31, 2020

@spacedmonkey @swissspidy @miina @pbakaus So for this issue, we need to use the font variant like the italic if exists. And no way to detect if the font variants exists or not currently. I can't see any fields that related with font variants from the fonts api. So I request api update for font variants. So basically I think a new field required that shows the available variants for this fonts. Like variants: ['Italic']

I'm super confused - we already have these variants... Like, we have them in the editor today:

image

For some reason this dropdown currently doesn't show the italic variants though (maybe we're already filtering them?).

@swissspidy
Copy link
Collaborator

We do have these variants in the dropdown, but they're hardcoded. Some fonts don't support things like super bold though. I think one thing discussed here is that the list should be dynamic depending on what the font supports (otherwise browser does handle boldness on its own).

However, there seems to be a misunderstanding here and this is not actually wanted. I think what we want here is that when pressing the Italic toggle, we need to make sure the Italic font variant is loaded accordingly.

@pbakaus can you perhaps clarify what was discussed here originally?

@pbakaus
Copy link
Contributor

pbakaus commented Mar 31, 2020

We do have these variants in the dropdown, but they're hardcoded. Some fonts don't support things like super bold though. I think one thing discussed here is that the list should be dynamic depending on what the font supports (otherwise browser does handle boldness on its own).

OMG! This list needs to be dynamic or up to date. Facepalm. Please somebody prioritize this.

However, there seems to be a misunderstanding here and this is not actually wanted. I think what we want here is that when pressing the Italic toggle, we need to make sure the Italic font variant is loaded accordingly.

@pbakaus can you perhaps clarify what was discussed here originally?

Yes, but that requires having access to the (real) variant list.

@swissspidy
Copy link
Collaborator

So to summarize this:

  • Font weight dropdown needs to be dynamic
    • This does not require an API change AFAIK. Font weights are exposed in the REST API & thus available in FontProvider
  • Optimize enqueueing Google fonts so only the required fonts weights and variants are actually loaded
  • We should handle enqueueing Google Fonts in the client-side generated AMP output for this (port \Google\Web_Stories\Story_Post_Type::load_fonts)

Note: not all fonts support font weights. For example, Abel only supports "regular". No bold. No italic. How should we display the Italic toggle and the font weights dropdown in that case? cc @samitron7

(If we need to disable the Italic toggle, this would need an API change, see #876 for that.)

@pbakaus
Copy link
Contributor

pbakaus commented Mar 31, 2020

So to summarize this:

  • Font weight dropdown needs to be dynamic

    • This does not require an API change AFAIK. Font weights are exposed in the REST API & thus available in FontProvider
  • Optimize enqueueing Google fonts so only the required fonts weights and variants are actually loaded

  • We should handle enqueueing Google Fonts in the client-side generated AMP output for this (port \Google\Web_Stories\Story_Post_Type::load_fonts)

Yes to all of that, and rather sooner than later. These are critical to get right.

Note: not all fonts support font weights. For example, Abel only supports "regular". No bold. No italic. How should we display the Italic toggle and the font weights dropdown in that case? cc @samitron7

They're still working normally - in this case, the italic and bold buttons default to the browser approximation (font-style: italic etc).

(If we need to disable the Italic toggle, this would need an API change, see #876 for that.)

@ndev1991
Copy link
Contributor

ndev1991 commented Apr 1, 2020

So to summarize this:

  • Font weight dropdown needs to be dynamic

    • This does not require an API change AFAIK. Font weights are exposed in the REST API & thus available in FontProvider
  • Optimize enqueueing Google fonts so only the required fonts weights and variants are actually loaded

  • We should handle enqueueing Google Fonts in the client-side generated AMP output for this (port \Google\Web_Stories\Story_Post_Type::load_fonts)

Yes to all of that, and rather sooner than later. These are critical to get right.

Note: not all fonts support font weights. For example, Abel only supports "regular". No bold. No italic. How should we display the Italic toggle and the font weights dropdown in that case? cc @samitron7

They're still working normally - in this case, the italic and bold buttons default to the browser approximation (font-style: italic etc).

(If we need to disable the Italic toggle, this would need an API change, see #876 for that.)

@pbakaus @swissspidy Here is the current implementation our version.
Get all the fonts list with available font-weights at the beginning by using api web-stories/v1/fonts and store the fonts list to fontProvider.
If user select text there is textStyle panel available with font family and font weight dropdown.
The font weights dropdown generated by using font family name from fontProvider and the available weights are from api. So actually the font weights dropdown is dynamic based on each font.
Once the user select different font that is not already loaded then it fetch that font from fontProvider and load that fonts with available font-weights.

And there are two types of bold at the moment. One through bold toggle and the other one is through font-weights dropdown.

So the only thing here is to sync those both I think. Basically when select the bold from font-weights dropdown it set the font-weights of that text. But when select the bold from toggle it uses strong tag.

To summarize.

  • All expected things already been implemented
  • The only thing to do with this ticket is to sync bold toggle and the font-weights (when the font has bold font weights)

@dvoytenko
Copy link
Contributor

From what I can tell, this task should involve:

  1. It looks like we already filter the complete set of weights with those defined for the font family. We just need to also use the weight names defined for the font family, and not the hard-coded names.
  2. When a new font family is selected, the closest match for the current font weight should be picked. (Might already be done, please check).

I added the following follow-up issues: #925, #923, #921, #922.

@ndev1991
Copy link
Contributor

ndev1991 commented Apr 1, 2020

From what I can tell, this task should involve:

  1. It looks like we already filter the complete set of weights with those defined for the font family. We just need to also use the weight names defined for the font family, and not the hard-coded names.
  2. When a new font family is selected, the closest match for the current font weight should be picked. (Might already be done, please check).

I added the following follow-up issues: #925, #923, #921, #922.

@dvoytenko Thanks for your clarification.
No 2. makes perfect sense for me. I will check that soon.
No 1. Yes, we get various available font-weights from font api at the moment. Here is the example.
name: "Baumans"
slug: "baumans"
handle: "baumans-font"
src: "https://fonts.googleapis.com/css?family=Baumans%3A400&subset=latin%2Clatin-ext&display=swap"
fallbacks: ["cursive"]
weights: ["400"]

But not way to get the correct weights names defined for the font family

@pbakaus
Copy link
Contributor

pbakaus commented Apr 1, 2020

It looks like the weight names are always the same. They are:

  • Thin (100)
  • Extra-light (200)
  • Light (300)
  • Regular (400)
  • Medium (500)
  • Semi-bold (600)
  • Bold (700)
  • Extra-bold (800)
  • Black (900)

@dvoytenko
Copy link
Contributor

Ok. So perhaps point (1) is already WAI? Just need to change the hardcoded names?

@pbakaus
Copy link
Contributor

pbakaus commented Apr 1, 2020

Yep.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
9 participants