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

Font size: design updates #36545

Closed
3 of 5 tasks
kellychoffman opened this issue Nov 16, 2021 · 22 comments
Closed
3 of 5 tasks

Font size: design updates #36545

kellychoffman opened this issue Nov 16, 2021 · 22 comments
Assignees
Labels
[Feature] Design Tools Tools that impact the appearance of blocks both to expand the number of tools and improve the experi Needs Design Feedback Needs general design feedback.

Comments

@kellychoffman
Copy link
Contributor

kellychoffman commented Nov 16, 2021

Design updates needed for 5.9

  • Add tooltips that show “medium”, “small”, etc. to the font size segmented control.
  • Scrolling issues with appearance dropdown (report). (discussion/PR)

Design updates proposed for 5.9

@kellychoffman kellychoffman added the Needs Design Needs design efforts. label Nov 16, 2021
@mtias mtias added the [Feature] Design Tools Tools that impact the appearance of blocks both to expand the number of tools and improve the experi label Nov 16, 2021
@javierarce
Copy link
Contributor

If I'm not mistaken, we are not showing the segmented controls because of two problems related to the theme.json:

  • We have a couple of items that use CSS properties to calculate their values (Large & Huge)
  • We have a couple of items that contain a dot inside the size (Normal & Medium)
  {
    "name": "Small",
    "size": "1rem",
    "slug": "small"
  },
  {
    "name": "Normal",
    "size": "1.125rem",
    "slug": "normal"
  },
  {
    "name": "Medium",
    "size": "1.75rem",
    "slug": "medium"
  },
  {
    "name": "Large",
    "size": "clamp(1.75rem, 3vw, 2.25rem)",
    "slug": "large"
  },
  {
    "name": "Huge",
    "size": "clamp(2.5rem, 4vw, 3rem)",
    "slug": "huge"
  }
]

The first problem will require some design changes to show that those two values contain custom values. The second one could be fixed in code.

I'll provide a design slightly based on my proposed solution for this issue

@javierarce
Copy link
Contributor

javierarce commented Nov 17, 2021

Here's a couple of ideas:

The first one is more complicated technically (and maybe impossible to do). It would require calculating a value (in the case of a clamp function, the lower limit) and showing it with an asterisk to indicate that it's a special kind of number (= dynamically calculated).

The second idea is easier to implement: we simply show the initial letter of the name. In case two labels start with the same name, we could append a number (L1, L2).

image

Related PR | Figma file

@javierarce javierarce added the Needs Design Feedback Needs general design feedback. label Nov 17, 2021
@kellychoffman kellychoffman removed the Needs Design Needs design efforts. label Nov 17, 2021
@kellychoffman
Copy link
Contributor Author

kellychoffman commented Nov 17, 2021

Nice!

Some feedback:

If we can do the first version, I would remove the *. Even though it's dynamically generated, I'm not sure its imperative the user knows that. Unless I am missing something.

For the second version, I think mixing numbers in with letters in the same UI is a bit too confusing. I would do one or the other. Something along the lines of:
Screen Shot 2021-11-17 at 8 32 00 PM
Screen Shot 2021-11-17 at 8 34 28 PM

@ndiego
Copy link
Member

ndiego commented Nov 18, 2021

@kellychoffman for the font size hints with decimal points, I already have a PR ready for that, just needs final approval: #36420 Obviously a more technical solution for calculating values that include var, clamp etc is needed down the line.

@Mamaduka
Copy link
Member

@javierarce, I'm moving this issue back to the To-do column. The "Needs Review" is mainly for PRs.

@javierarce
Copy link
Contributor

@javierarce, I'm moving this issue back to the To-do column. The "Needs Review" is mainly for PRs.

Oh, thanks @Mamaduka, I didn't know that! 😓


Even though it's dynamically generated, I'm not sure it's imperative the user knows that. Unless I am missing something.

The only problem I see with that approach (showing a number that was dynamically calculated without any indication of its nature) is that a user could select 48, visit the site and see a totally different size. Also, the result of calculating the dynamic value could be smaller or equal to any of the fixed values. So that's why I think it's important to indicate when those values are calculated.

For the second version, I think mixing numbers in with letters in the same UI is a bit too confusing.

True. The all-initials approach makes more sense to me, although I would still indicate that L & H are dynamic on the hint label.

I guess that the logic for the segment control would then be this one:

  • If all the values are fixed, show numbers.
  • If any of the values are dynamic, show initials.

Also, maybe the theme.json file could also have a property to force the second mode.

@ntsekouras
Copy link
Contributor

There are only 5-6 font sizes but the styles sidebar still shows a big dropdown. Make sure Twenty twenty two theme uses the segmented control for font sizes.

This cannot be done, because now we don't have a way of calculating dynamic values (css vars, clump, etc...). So if we have at least one such value we show the select control. The current check for displaying the segmented control is if we have less than five options and if all of them are simple css values(no dynamic calculation).

The proper solution is to calculate the dynamic values which is not really simple. @jorgefilipecosta I think had some PR about this..

An alternative would be to show a segmented control with labels, but this will need to take into account other things, because these labels could be too big and make the control incomprehensible, as we have limited space in

“Default” should not be an option. Unsetting styles should be done with the reset button.

“Custom” should not be an option in the dropdown given there is a specific button for showing the custom size controls.

Both these values (default, custom) exist because for both cases if we switch back to the select control, we should have an available option in the select control. For example if we unset through the reset button, what will be shown? Then if we have a custom value and toggle to maybe select something else from the available font sizes, something should be shown there as well.

@ndiego
Copy link
Member

ndiego commented Nov 18, 2021

Devil's advocate here, sorry 😉

Given the complexity of displaying a value that would be recognizable to the user, perhaps 5.9 just sticks with the dropdown regardless of the amount of Font Sizes registered? With a few minor tweaks to the "hits", as discussed here #36420, I personally think that would be sufficient. The dropdown may not be perfect, but it is easy grok with the clear names "Small, Medium, Large, etc.", especially for newer users.

@ramonjd
Copy link
Member

ramonjd commented Nov 18, 2021

Both these values (default, custom) exist because for both cases if we switch back to the select control, we should have an available option in the select control. For example if we unset through the reset button, what will be shown? Then if we have a custom value and toggle to maybe select something else from the available font sizes, something should be shown there as well.

Agree here.

At first my mind turned to select controls with a "Please select" item at the top of the options list.

Selecting that option after having already chosen a font size would, I imagine, have the same effect as the current "Default".

The "Default" label, at least, communicates what is going to happen when we select it.

Unless... the "Please select" item only appears after unsetting via a "Reset" button (?).

@aaronrobertshaw
Copy link
Contributor

There's an alternate approach in #36636 that builds upon the work by @ndiego in #36420. I kept it as a separate PR in case I was on the wrong track.

The PR linked:

  • Removes size hints from the select control options
  • Allows for non-integer font-size values such as 1.5rem to be displayed in the control's label/header hint
  • Also allows non-integer values that are < 1. e.g. 0.75em in header hint
  • Displays "dynamic" in the header hint for complex CSS values such as clamp(2.5rem, 4vw, 3rem)
Storybook Editor
FontSizePickerStory FontSizePickerEditor2022

@ramonjd
Copy link
Member

ramonjd commented Nov 22, 2021

I've thrown up an attempt to show tooltips on the ToggleGroupControlOption in #36726. Adding to Needs Review column as well.

@apeatling apeatling moved this from ⏱ Not Started to ⏳ In Progress in Design Tools Project Nov 23, 2021
@oandregal
Copy link
Member

Another thing I've run into is that the appearance dropdown in the GlobalStyles sidebar is cut by some scrolling issues (Firefox browser, Linux operating system):

Captura de ecrã de 2021-11-23 13-03-43

@ntsekouras
Copy link
Contributor

There are only 5-6 font sizes but the styles sidebar still shows a big dropdown. Make sure Twenty twenty two theme uses the segmented control for font sizes.

I'll have a PR about this today with an approach we can discuss.

@mtias
Copy link
Member

mtias commented Dec 3, 2021

The main item that's important for 5.9 is #37038, so once that is merged we can consider this concluded for 5.9. Further improvements can land later.

@jorgefilipecosta
Copy link
Member

The proper solution is to calculate the dynamic values which is not really simple. @jorgefilipecosta I think had some PR about this..

As we (@ntsekouras and I) talked on a call, it is not possible to calculate all the dynamic values reliably. They may depend on the device of the viewer etc... We can, and ideally should correctly display some dynamic values like CSS variable references that were defined on theme.json.

I think if a font-size is set to "clamp(1.8rem, 2.5vw, 2.8rem)" our UI should show something useful to most users e.g., "Large" or "L". But ideally, an advanced user could see in a raw text field the font size is "clamp(1.8rem, 2.5vw, 2.8rem)" and be able to make adjustments.

@ntsekouras
Copy link
Contributor

ntsekouras commented Dec 21, 2021

I think all the related PRs here got merged. Should we close this one?

@mtias mtias closed this as completed Dec 21, 2021
Repository owner moved this from ⏳ In Progress to ✅ Done in Design Tools Project Dec 21, 2021
@uniquesolution
Copy link

Hi, @aaronrobertshaw , I want to suggest that instead of just showing dynamic text for "clamp(1.8rem, 2.5vw, 2.8rem)" type of font size, I think it should show min max or either just the max value together with it, something like "Dynamic - 2.8rem)" because, currently it become difficult to get idea what font size text is taking.

@ntsekouras
Copy link
Contributor

because, currently it become difficult to get idea what font size text is taking.

👋 - the use cases can vary a lot like clamp, var etc.. and even if we extracted something from everything It's not really indicative of the final font size. This is especially true in general for relative length units like em, rem, etc...

@uniquesolution
Copy link

@ntsekouras , then I suggest, show font size as tooltip on hover and it should simply display the value we exactly put in the theme.json file, or from global settings.

@ntsekouras
Copy link
Contributor

@ntsekouras , then I suggest, show font size as tooltip on hover and it should simply display the value we exactly put in the theme.json file, or from global settings.

That was the reason behind these changes. Do you think it would be helpful to show for example var(--some-variable-name)?

@uniquesolution
Copy link

@ntsekouras , then I suggest, show font size as tooltip on hover and it should simply display the value we exactly put in the theme.json file, or from global settings.

That was the reason behind these changes. Do you think it would be helpful to show for example var(--some-variable-name)?

In case of variable, we can show the value of variable , (for example --wp--preset--font-size--my-font-size: 2rem; should show 2rem for var(--wp--preset--font-size--my-font-size), but in case of any fixed value, like 2rem, 12px, clamp(16px, 2vw, 20px) etc, it should show exact the same value as provided. for common person it gives the idea how the font size is taking its value.

@aaronrobertshaw
Copy link
Contributor

Thanks for the suggestions and thoughts @uniquesolution, they are appreciated 👍

I think I am still leaning towards omitting any additional information that could confuse average users. The majority of users would not understand clamp(16px, 2vw, 20px) etc. CSS custom properties could also have complex values themselves e.g. calc(var(--wp--some--other--var) + 10px)). I suspect that any users advanced enough to understand these and want to know their value would also already be inspecting values through dev tools.

There was some discussion on one of the PRs related to this, that might shine some light on how we ended up with the current behaviour.

If you are interested in putting together a proof of concept PR with any further ideas I'd be more than happy to help review!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Design Tools Tools that impact the appearance of blocks both to expand the number of tools and improve the experi Needs Design Feedback Needs general design feedback.
Projects
Status: Done
Development

No branches or pull requests