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

Predefined set of font sizes #5269

Merged
merged 14 commits into from Mar 20, 2018

Conversation

Projects
None yet
5 participants
@mtias
Contributor

mtias commented Feb 26, 2018

Allow font-size control to have a predefined set of sizes (small, regular, large, larger) which applies classes instead of inline font-size style values. If the value does not correspond to a value in the supplied set, it falls back to previous mechanism.

Themes will be able to define their own set to overwrite the default.

The design aim would be to add a new component (segmented control) which we could use for the image size set as well.

Implementation

  • Introduce new ButtonGroup component.
  • Add notion of text class to paragraph block.
  • Define predefined set of font sizes.
  • If a font size matches a class, use class instead of inline style.

Current State

image

@jasmussen

This comment has been minimized.

Show comment
Hide comment
@jasmussen

jasmussen Feb 27, 2018

Contributor

I like the arbitrary font size slider, it allows for almost illustration-like layouts and designs, especially combined with colors. Although the slider can feel like a heavy-handed interface for adjusting things, in this particular case, I find it an enjoyable UI, because it can quickly cross a very wide range of sizes, and with the live preview on the left this can make for a pretty magical experience.

It feels like the challenge in this case is the confusion around inline styles and classes, and making it obvious when either is used. This is a difficult UI. While one solution could be to remove the slider in favor of an input field, this doesn't solve the particular issue of clarifying to the user when a value is set and customized vs. when the value is intrinsic or default.

The closest pattern to this, is images. Images work fine when their width and heights are unset — sometimes that's what you want. But other times you want to manually configure those dimensions. Matías you even mentioned the pill-button shortcut UI for the ticket to address that: #4914 (comment).

To see how to improve the UI, I decided to try and tackle both font size, and image dimensions, to see if the pattern could be replicated. As part of that I looked at some improvements to the slider design itself, also embracing the pill button shortcuts.

screen shot 2018-02-27 at 10 40 10

Notes:

  • There's an extra A icon, larger than the left one, to indicate the relationship between small and big sizes.
  • When the font size is undefined, the actual font size is shown as a placeholder, in this case 13. It reflects the size of the text, but also by being grayed, that this size isn't explicitly set.
  • In addition to showing the unset size, we show the slider handle in the correct position that would correspond to the default value also.
  • Pressing the Reset button will always return the slider to that position, and unset the number so the default value is shown again as a placeholder.
  • If you pull the slider, or type a number in the stepper, you set the font size to a non-default value.
  • Below the slider there are shortcuts in a pill button. These are simply just that, shortcuts to set the value above. Note how 24 is pressed, and value is input above, and the slider is also set.

This "shortcut" pattern, I tried replicating for images as well, where the intrinsic values for an image (when not sized) are shown as placeholder text, and a press of the shortcut button to 100% sets those values explicitly.

Thoughts? CC: @mtias


Sidenotes: I noticed that GoDaddy's site builder has a slider for font sizes also, though it's for the whole document:

godaddy sizer

If we feel the slider is being too widely used, we can eliminate it in places where the precision control isn't quite as valuable. Perhaps it's not the ideal interface for the gallery columns interface. Maybe that could be just a stepper, perhaps with a little CSS to make the up/down buttons a little bigger and friendlier.

Contributor

jasmussen commented Feb 27, 2018

I like the arbitrary font size slider, it allows for almost illustration-like layouts and designs, especially combined with colors. Although the slider can feel like a heavy-handed interface for adjusting things, in this particular case, I find it an enjoyable UI, because it can quickly cross a very wide range of sizes, and with the live preview on the left this can make for a pretty magical experience.

It feels like the challenge in this case is the confusion around inline styles and classes, and making it obvious when either is used. This is a difficult UI. While one solution could be to remove the slider in favor of an input field, this doesn't solve the particular issue of clarifying to the user when a value is set and customized vs. when the value is intrinsic or default.

The closest pattern to this, is images. Images work fine when their width and heights are unset — sometimes that's what you want. But other times you want to manually configure those dimensions. Matías you even mentioned the pill-button shortcut UI for the ticket to address that: #4914 (comment).

To see how to improve the UI, I decided to try and tackle both font size, and image dimensions, to see if the pattern could be replicated. As part of that I looked at some improvements to the slider design itself, also embracing the pill button shortcuts.

screen shot 2018-02-27 at 10 40 10

Notes:

  • There's an extra A icon, larger than the left one, to indicate the relationship between small and big sizes.
  • When the font size is undefined, the actual font size is shown as a placeholder, in this case 13. It reflects the size of the text, but also by being grayed, that this size isn't explicitly set.
  • In addition to showing the unset size, we show the slider handle in the correct position that would correspond to the default value also.
  • Pressing the Reset button will always return the slider to that position, and unset the number so the default value is shown again as a placeholder.
  • If you pull the slider, or type a number in the stepper, you set the font size to a non-default value.
  • Below the slider there are shortcuts in a pill button. These are simply just that, shortcuts to set the value above. Note how 24 is pressed, and value is input above, and the slider is also set.

This "shortcut" pattern, I tried replicating for images as well, where the intrinsic values for an image (when not sized) are shown as placeholder text, and a press of the shortcut button to 100% sets those values explicitly.

Thoughts? CC: @mtias


Sidenotes: I noticed that GoDaddy's site builder has a slider for font sizes also, though it's for the whole document:

godaddy sizer

If we feel the slider is being too widely used, we can eliminate it in places where the precision control isn't quite as valuable. Perhaps it's not the ideal interface for the gallery columns interface. Maybe that could be just a stepper, perhaps with a little CSS to make the up/down buttons a little bigger and friendlier.

@jasmussen

This comment has been minimized.

Show comment
Hide comment
@jasmussen

jasmussen Feb 28, 2018

Contributor

Here's a version that uses T-shirt size verbiage for the buttons. I think it works well, and this would translate better to themes providing their own values for each of those buttons:

screen shot 2018-02-28 at 09 09 26

Contributor

jasmussen commented Feb 28, 2018

Here's a version that uses T-shirt size verbiage for the buttons. I think it works well, and this would translate better to themes providing their own values for each of those buttons:

screen shot 2018-02-28 at 09 09 26

@mtias

This comment has been minimized.

Show comment
Hide comment
@mtias

mtias Feb 28, 2018

Contributor

What about placing the sizes at the top of the slider? I see the relationship as "basic -> advanced". Same as with colors.

Contributor

mtias commented Feb 28, 2018

What about placing the sizes at the top of the slider? I see the relationship as "basic -> advanced". Same as with colors.

text-transform: uppercase;
font-style: normal;
p {
&.is-small-text {

This comment has been minimized.

@jorgefilipecosta

jorgefilipecosta Mar 8, 2018

Member

Should this classes (is-small-text, is-regular-text) etc... be outside of paragraph so other blocks can make use of our font size mechanism?

@jorgefilipecosta

jorgefilipecosta Mar 8, 2018

Member

Should this classes (is-small-text, is-regular-text) etc... be outside of paragraph so other blocks can make use of our font size mechanism?

This comment has been minimized.

@mtias

mtias Mar 8, 2018

Contributor

maybe, but I'm cautious about defining styles generically for *.is-small-text. Also, in some other blocks reusing this component, they might want to have more fine-grained targets.

@mtias

mtias Mar 8, 2018

Contributor

maybe, but I'm cautious about defining styles generically for *.is-small-text. Also, in some other blocks reusing this component, they might want to have more fine-grained targets.

This comment has been minimized.

@aduth

aduth Mar 9, 2018

Member

So this is a problem because it is applying outside the paragraph block. There's no scoping on the p selector to make this specific to the paragraph block.

@aduth

aduth Mar 9, 2018

Member

So this is a problem because it is applying outside the paragraph block. There's no scoping on the p selector to make this specific to the paragraph block.

@@ -46,6 +49,13 @@ const ContrastCheckerWithFallbackStyles = withFallbackStyles( ( node, ownProps )
};
} )( ContrastChecker );
const FONT_SIZES = {

This comment has been minimized.

@jorgefilipecosta

jorgefilipecosta Mar 8, 2018

Member

Should we try to read this values from the theme, e.g: provide and extensibility mechanism for themes to change them, or use invisible elements plus getComputedStyles to check if themes css changes our styles?

@jorgefilipecosta

jorgefilipecosta Mar 8, 2018

Member

Should we try to read this values from the theme, e.g: provide and extensibility mechanism for themes to change them, or use invisible elements plus getComputedStyles to check if themes css changes our styles?

This comment has been minimized.

@mtias

mtias Mar 8, 2018

Contributor

definitely allow a theme to set this, but should be done separately

@mtias

mtias Mar 8, 2018

Contributor

definitely allow a theme to set this, but should be done separately

@jasmussen

This comment has been minimized.

Show comment
Hide comment
@jasmussen

jasmussen Mar 8, 2018

Contributor

This is working EXCEPTIONALLY well.

I have only ONE tiny niggle, which is best illustrated in this GIF:

textsettings

At the end when I press Reset, the slider handle is centered in the slider. Which, in context of just having resized the text, makes it seem like suddenly the text should be large (because of the slider handle position).

I realize that the handle at the center is the "unset" configuration for the element: https://developer.mozilla.org/en-US/docs/Web/HTML/Element/input/range — but is there something we can do to indicate that the slider is unset?

Contributor

jasmussen commented Mar 8, 2018

This is working EXCEPTIONALLY well.

I have only ONE tiny niggle, which is best illustrated in this GIF:

textsettings

At the end when I press Reset, the slider handle is centered in the slider. Which, in context of just having resized the text, makes it seem like suddenly the text should be large (because of the slider handle position).

I realize that the handle at the center is the "unset" configuration for the element: https://developer.mozilla.org/en-US/docs/Web/HTML/Element/input/range — but is there something we can do to indicate that the slider is unset?

@mtias

This comment has been minimized.

Show comment
Hide comment
@mtias

mtias Mar 8, 2018

Contributor

I realize that the handle at the center is the "unset" configuration for the element: https://developer.mozilla.org/en-US/docs/Web/HTML/Element/input/range — but is there something we can do to indicate that the slider is unset?

We could try hiding the disc. Once you click on the bar, it would set it again.

Contributor

mtias commented Mar 8, 2018

I realize that the handle at the center is the "unset" configuration for the element: https://developer.mozilla.org/en-US/docs/Web/HTML/Element/input/range — but is there something we can do to indicate that the slider is unset?

We could try hiding the disc. Once you click on the bar, it would set it again.

@jasmussen

This comment has been minimized.

Show comment
Hide comment
@jasmussen

jasmussen Mar 8, 2018

Contributor

Or making it the same gray as the track maybe. Or maybe it's a non issue... You'll learn what reset means by the context anyway.

Contributor

jasmussen commented Mar 8, 2018

Or making it the same gray as the track maybe. Or maybe it's a non issue... You'll learn what reset means by the context anyway.

text-transform: uppercase;
font-style: normal;
p {
&.is-small-text {

This comment has been minimized.

@aduth

aduth Mar 9, 2018

Member

So this is a problem because it is applying outside the paragraph block. There's no scoping on the p selector to make this specific to the paragraph block.

@aduth

aduth Mar 9, 2018

Member

So this is a problem because it is applying outside the paragraph block. There's no scoping on the p selector to make this specific to the paragraph block.

<PanelBody title={ __( 'Text Settings' ) }>
<PanelBody title={ __( 'Text Settings' ) } className="blocks-font-size">
<div className="blocks-font-size__main">
<ButtonGroup aria-label={ __( 'Font Size' ) }>

This comment has been minimized.

@aduth

aduth Mar 9, 2018

Member

Won't we want aria-label on each of the button options instead for "Small", "Medium", etc. ?

@aduth

aduth Mar 9, 2018

Member

Won't we want aria-label on each of the button options instead for "Small", "Medium", etc. ?

This comment has been minimized.

@mtias

mtias Mar 9, 2018

Contributor

We discussed this and opted for not having one for now.

@mtias

mtias Mar 9, 2018

Contributor

We discussed this and opted for not having one for now.

Show outdated Hide outdated blocks/library/paragraph/index.js
const classes = classnames( 'components-button-group', className );
return (
<div { ...props } className={ classes } role="group" />

This comment has been minimized.

@aduth

aduth Mar 9, 2018

Member

A group should be used to form a logical collection of items with related functionality, such as children in a tree widget forming a collection of siblings in a hierarchy, or a collection of items having the same container in a directory. However, when a group is used in the context of list, authors must limit its children to listitem elements. Group elements may be nested.

https://developer.mozilla.org/en-US/docs/Web/Accessibility/ARIA/ARIA_Techniques/Using_the_group_role

Maybe we need to Children.map to apply a wrapper to each child?

@aduth

aduth Mar 9, 2018

Member

A group should be used to form a logical collection of items with related functionality, such as children in a tree widget forming a collection of siblings in a hierarchy, or a collection of items having the same container in a directory. However, when a group is used in the context of list, authors must limit its children to listitem elements. Group elements may be nested.

https://developer.mozilla.org/en-US/docs/Web/Accessibility/ARIA/ARIA_Techniques/Using_the_group_role

Maybe we need to Children.map to apply a wrapper to each child?

This comment has been minimized.

@mtias

mtias Mar 19, 2018

Contributor

Discussed in slack channel and markup seemed good as is.

@mtias

mtias Mar 19, 2018

Contributor

Discussed in slack channel and markup seemed good as is.

Show outdated Hide outdated blocks/library/paragraph/index.js
Show outdated Hide outdated blocks/library/paragraph/index.js
@@ -117,21 +144,63 @@ class ParagraphBlock extends Component {
),
isSelected && (
<InspectorControls key="inspector">
<PanelBody title={ __( 'Text Settings' ) }>
<PanelBody title={ __( 'Text Settings' ) } className="blocks-font-size">

This comment has been minimized.

@aduth

aduth Mar 9, 2018

Member

I expect we're planning to extract this to a separate component?

@aduth

aduth Mar 9, 2018

Member

I expect we're planning to extract this to a separate component?

This comment has been minimized.

@mtias

mtias Mar 9, 2018

Contributor

Indeed.

@mtias

mtias Mar 9, 2018

Contributor

Indeed.

@@ -1,3 +1,13 @@
.editor-block-list__block:not( .is-multi-selected ) .wp-block-paragraph {
background: white;
}
.blocks-font-size__main {

This comment has been minimized.

@mtias

mtias Mar 19, 2018

Contributor

This will be extracted into a block component.

@mtias

mtias Mar 19, 2018

Contributor

This will be extracted into a block component.

Show outdated Hide outdated blocks/library/paragraph/index.js
}
&.has-drop-cap {
&:first-letter {

This comment has been minimized.

@aduth

aduth Mar 19, 2018

Member

Nesting necessary? vs.

&.has-drop-cap:first-letter {

}
@aduth

aduth Mar 19, 2018

Member

Nesting necessary? vs.

&.has-drop-cap:first-letter {

}
@youknowriad

This comment has been minimized.

Show comment
Hide comment
@youknowriad

youknowriad Mar 20, 2018

Contributor

After taking a look more deeply to the latest changes, I noticed an issue:

An old paragraph with a custom font size is still valid even if the markup is different.
The reason it’s still valid is tricky. Because if you take a look at the generated markup it’s different than the saved one (the font-size style is missing from the new block). But the block is not marked as invalid because we have a deprecated version that doesn’t care about the content (the one that handles p-less paragraphs)

So I think we have two options:

  • Drop the custom logic to handle old fontSize values in getFontSize because it makes you think the font size is customized but it's not really in the frontend.

  • Add a new deprecated version for the paragraph block to migrate the old fontSize to customFontSize.

Contributor

youknowriad commented Mar 20, 2018

After taking a look more deeply to the latest changes, I noticed an issue:

An old paragraph with a custom font size is still valid even if the markup is different.
The reason it’s still valid is tricky. Because if you take a look at the generated markup it’s different than the saved one (the font-size style is missing from the new block). But the block is not marked as invalid because we have a deprecated version that doesn’t care about the content (the one that handles p-less paragraphs)

So I think we have two options:

  • Drop the custom logic to handle old fontSize values in getFontSize because it makes you think the font size is customized but it's not really in the frontend.

  • Add a new deprecated version for the paragraph block to migrate the old fontSize to customFontSize.

jorgefilipecosta added some commits Mar 19, 2018

@jorgefilipecosta

This comment has been minimized.

Show comment
Hide comment
@jorgefilipecosta

jorgefilipecosta Mar 20, 2018

Member

Hi @youknowriad, this PR now uses the deprecation mechanism to convert existing blocks. The problem you described should be fixed.

Member

jorgefilipecosta commented Mar 20, 2018

Hi @youknowriad, this PR now uses the deprecation mechanism to convert existing blocks. The problem you described should be fixed.

@youknowriad

This comment has been minimized.

Show comment
Hide comment
@youknowriad

youknowriad Mar 20, 2018

Contributor

I'll defer to @mtias here. While this fixes the problem, it seems that the default block's implementation is getting bigger and bigger just to handle legacy issues. While that's totally fine once Gutenberg merged into Core, it's good to try to limit it while not done yet.

Maybe it's fine to keep it for a release or two and then remove it later.

Contributor

youknowriad commented Mar 20, 2018

I'll defer to @mtias here. While this fixes the problem, it seems that the default block's implementation is getting bigger and bigger just to handle legacy issues. While that's totally fine once Gutenberg merged into Core, it's good to try to limit it while not done yet.

Maybe it's fine to keep it for a release or two and then remove it later.

@jorgefilipecosta

This comment has been minimized.

Show comment
Hide comment
@jorgefilipecosta

jorgefilipecosta Mar 20, 2018

Member

I'll defer to @mtias here. While this fixes the problem, it seems that the default block's implementation is getting bigger and bigger just to handle legacy issues. While that's totally fine once Gutenberg merged into Core, it's good to try to limit it while not done yet.

Maybe it's fine to keep it for a release or two and then remove it later.

Totally agree here, my expectation is that it would follow our normal deprecation rules. And stay there two releases, as soon as an existing post is open this code coverts it. So most users will not be affected by this change.

Member

jorgefilipecosta commented Mar 20, 2018

I'll defer to @mtias here. While this fixes the problem, it seems that the default block's implementation is getting bigger and bigger just to handle legacy issues. While that's totally fine once Gutenberg merged into Core, it's good to try to limit it while not done yet.

Maybe it's fine to keep it for a release or two and then remove it later.

Totally agree here, my expectation is that it would follow our normal deprecation rules. And stay there two releases, as soon as an existing post is open this code coverts it. So most users will not be affected by this change.

@mtias

This comment has been minimized.

Show comment
Hide comment
@mtias

mtias Mar 20, 2018

Contributor

Let's keep it then but make sure to remove in a couple releases.

Contributor

mtias commented Mar 20, 2018

Let's keep it then but make sure to remove in a couple releases.

@youknowriad

LGTM 👍

@mtias mtias merged commit f4cc3bf into master Mar 20, 2018

2 checks passed

codecov/project 44.22% (+0.01%) compared to aefb285
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@mtias mtias deleted the add/predefined-font-sizes branch Mar 20, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment