1472966 - Font editor: add style tweaks to bring the UI closer to the latest designs

Status: RESOLVED FIXED

(DevTools :: Inspector, enhancement, P3)

Description

[Razvan Caliman [:rcaliman]]

https://mozilla.invisionapp.com/share/GMMC9JHERS2#/screens

- Cap list of used fonts at 3 items and provide toggle to reveal more
- Hide font name if identical to font family name
- Tweak sliders and remove duplicate focus rings from inputs
- Tweak spacing and borders

Comment 1

[Razvan Caliman [:rcaliman]]

Attachment: Bug 1472966 - Styling tweaks for Font Editor.

Review commit: https://reviewboard.mozilla.org/r/254532/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/254532/

Comment 2

[Razvan Caliman [:rcaliman]]

Attachment: Bug 1472966 - Replace label for list of unused font families.

Review commit: https://reviewboard.mozilla.org/r/254534/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/254534/

Comment 3

[Razvan Caliman [:rcaliman]]

Attachment: Bug 1472966 - Limit list of used fonts to 3 and put others in a collapsed list.

Review commit: https://reviewboard.mozilla.org/r/254536/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/254536/

Comment 4

[Razvan Caliman [:rcaliman]]

Attachment: Bug 1472966 - Do not show "Rendered fonts" accordion if there are no fonts to show.

Review commit: https://reviewboard.mozilla.org/r/254538/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/254538/

Comment 5

[Razvan Caliman [:rcaliman]]

Attachment: hide-font-name-if-equals-family-name.mp4

Resolved **not** to hide font name if it's the same as font family. When changing font properties, the used font may change (ex: Courier -> Courier Bold) and displaying the previously hidden element shifts the UI vertically.

Comment 6

[Gabriel Luong [:gl] (ΦωΦ)]

Comment on attachment 8989503 [details]
Bug 1472966 - Do not show "Rendered fonts" accordion if there are no fonts to show.

https://reviewboard.mozilla.org/r/254538/#review261464

Comment 7

[Gabriel Luong [:gl] (ΦωΦ)]

Comment on attachment 8989502 [details]
Bug 1472966 - Limit list of used fonts to 3 and put others in a collapsed list.

https://reviewboard.mozilla.org/r/254536/#review261466

::: devtools/client/inspector/fonts/components/FontEditor.js:123
(Diff revision 1)
>        return null;
>      }
>  
> -    const fontList = dom.ul(
> -      {
> -        className: "fonts-list"
> +    // Maximum number of font families to be shown by default. Any others will be hidden
> +    // under a collapsed <details> element with a toggle to reveal them.
> +    const MAX_FONTS = 3;

Let's move this MAX_FONTS constant out of the class  and below the requires()

::: devtools/client/inspector/fonts/components/FontEditor.js:174
(Diff revision 1)
> +
> +    return dom.ul(
> +      {
> +        className: "fonts-list"
> +      },
> +      fonts.map(font => {

You want to set a key for these situations with React.

::: devtools/client/inspector/fonts/components/FontEditor.js:177
(Diff revision 1)
> +        className: "fonts-list"
> +      },
> +      fonts.map(font => {
> +        return dom.li(
> +          {},
> +          FontMeta({ font, onToggleFontHighlight: this.props.onToggleFontHighlight })

Would prefer if the props are on their own lines for better readability when there are >= 2 props

Comment 8

[Gabriel Luong [:gl] (ΦωΦ)]

Comment on attachment 8989501 [details]
Bug 1472966 - Replace label for list of unused font families.

https://reviewboard.mozilla.org/r/254534/#review261468

Comment 9

[Gabriel Luong [:gl] (ΦωΦ)]

Comment on attachment 8989500 [details]
Bug 1472966 - Styling tweaks for Font Editor.

https://reviewboard.mozilla.org/r/254532/#review261470

::: devtools/client/themes/fonts.css:186
(Diff revision 1)
> +.font-value-input:-moz-ui-invalid {
> +  box-shadow: none;
> +}
> +
> +/* Do not show dotted line focus outline */
> +.font-value-input:-moz-focusring {

Do we actually want to remove the focus outline for inputs? Seems weird since we kinda keep them everywhere.

::: devtools/client/themes/fonts.css:225
(Diff revision 1)
>    /* Offset the background so the notch aligns with the center of the slider thumb */
> -  background-position: 7px center;
> +  background-position: 5px center;
>    /* Repeat the background-image horizontally */
>    background-repeat: repeat-x;
>    /* Size the background to get nine visible notch instances. */
> -  background-size: calc(12.5% - var(--notch-size)) 90%;
> +  background-size: calc(12.5% - var(--notch-size)/2) 7px;

s/var(--notch-size)/2/var(--notch-size) / 2

Add some spacing besides the "/"

Comment 10

[Razvan Caliman [:rcaliman]]

Attachment: Bug 1472966 - Miscellaneous tweaks to paddings, margins and borders for font editor.

Review commit: https://reviewboard.mozilla.org/r/254728/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/254728/

Comment 11

[Razvan Caliman [:rcaliman]]

Comment on attachment 8989500 [details]
Bug 1472966 - Styling tweaks for Font Editor.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/254532/diff/1-2/

Comment 12

[Razvan Caliman [:rcaliman]]

Comment on attachment 8989501 [details]
Bug 1472966 - Replace label for list of unused font families.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/254534/diff/1-2/

Comment 13

[Razvan Caliman [:rcaliman]]

Comment on attachment 8989502 [details]
Bug 1472966 - Limit list of used fonts to 3 and put others in a collapsed list.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/254536/diff/1-2/

Comment 14

[Razvan Caliman [:rcaliman]]

Comment on attachment 8989503 [details]
Bug 1472966 - Do not show "Rendered fonts" accordion if there are no fonts to show.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/254538/diff/1-2/

Comment 15

[Razvan Caliman [:rcaliman]]

Comment on attachment 8989500 [details]
Bug 1472966 - Styling tweaks for Font Editor.

https://reviewboard.mozilla.org/r/254532/#review261470

> Do we actually want to remove the focus outline for inputs? Seems weird since we kinda keep them everywhere.

This still keeps the standard focus ring. It only removes a duplicate focus ring made up of a dotted inner outline on the input box.

Comment 16

[Gabriel Luong [:gl] (ΦωΦ)]

Comment on attachment 8989723 [details]
Bug 1472966 - Miscellaneous tweaks to paddings, margins and borders for font editor.

https://reviewboard.mozilla.org/r/254728/#review261600

::: devtools/client/inspector/fonts/components/FontEditor.js:61
(Diff revision 1)
>  
>      return step.toString();
>    }
>  
>    /**
> -   * Get an array of FontPropertyValue components for of the given variable font axes.
> +   * Get a container with rendered FontPropertyValue components with editing controls

Wording:
Get a container with the rendered..

::: devtools/client/inspector/fonts/components/FontEditor.js:62
(Diff revision 1)
>      return step.toString();
>    }
>  
>    /**
> -   * Get an array of FontPropertyValue components for of the given variable font axes.
> -   * If an axis is defined in the fontEditor store, use its value, else use the default.
> +   * Get a container with rendered FontPropertyValue components with editing controls
> +   * for of the given variable font axes. If no axes were given, return null.

for a given variable font axes.

::: devtools/client/inspector/fonts/components/FontEditor.js:64
(Diff revision 1)
>  
>    /**
> -   * Get an array of FontPropertyValue components for of the given variable font axes.
> -   * If an axis is defined in the fontEditor store, use its value, else use the default.
> +   * Get a container with rendered FontPropertyValue components with editing controls
> +   * for of the given variable font axes. If no axes were given, return null.
> +   * If an axis has a value in the fontEditor store (i.e.: it was declared in CSS or
> +   * it was changed using the font editor), use its value, else use the font axis default.

s/else/otherwise

Comment 17

[Razvan Caliman [:rcaliman]]

Comment on attachment 8989723 [details]
Bug 1472966 - Miscellaneous tweaks to paddings, margins and borders for font editor.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/254728/diff/1-2/

Comment 18

[Razvan Caliman [:rcaliman]]

Comment on attachment 8989723 [details]
Bug 1472966 - Miscellaneous tweaks to paddings, margins and borders for font editor.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/254728/diff/2-3/

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/392a0cb503d7
Styling tweaks for Font Editor. r=gl
https://hg.mozilla.org/integration/autoland/rev/39716d0d8c8b
Replace label for list of unused font families. r=gl
https://hg.mozilla.org/integration/autoland/rev/a6d7b6120c99
Limit list of used fonts to 3 and put others in a collapsed list. r=gl
https://hg.mozilla.org/integration/autoland/rev/fad612287694
Do not show "Rendered fonts" accordion if there are no fonts to show. r=gl
https://hg.mozilla.org/integration/autoland/rev/090812ea6e03
Miscellaneous tweaks to paddings, margins and borders for font editor. r=gl

Comment 20

[Dorel Luca [:dluca]]

https://hg.mozilla.org/mozilla-central/rev/392a0cb503d7
https://hg.mozilla.org/mozilla-central/rev/39716d0d8c8b
https://hg.mozilla.org/mozilla-central/rev/a6d7b6120c99
https://hg.mozilla.org/mozilla-central/rev/fad612287694
https://hg.mozilla.org/mozilla-central/rev/090812ea6e03

Comment 21

[Francesco Lodolo [:flod]]

Comment on attachment 8989501 [details]
Bug 1472966 - Replace label for list of unused font families.

https://reviewboard.mozilla.org/r/254534/#review261744

::: devtools/client/locales/en-US/font-inspector.properties:66
(Diff revision 2)
>  # in the font editor which allows the user to change the style of the font to italic.
>  fontinspector.fontItalicLabel=Italic
>  
> -# # LOCALIZATION NOTE (fontinspector.familiesNotUsedLabel): Semi-colon list of
> -# plural forms.
> -# See: http://developer.mozilla.org/en/docs/Localization_and_Plurals
> +# LOCALIZATION NOTE (fontinspector.familiesNotUsedLabel): Label for the list in the font
> +# editor which contains the names of font families declared but not used.
> +fontinspector.familiesNotUsedLabel=Unused font families

This is not allowed.
https://developer.mozilla.org/en-US/docs/Mozilla/Localization/Localization_content_best_practices#Changing_existing_strings

Please fix as soon as possible, otherwise I'll need to ask for a backout.

Also, please make sure to read the document linked above.

Comment 22

[Francesco Lodolo [:flod]]

Comment on attachment 8989502 [details]
Bug 1472966 - Limit list of used fonts to 3 and put others in a collapsed list.

https://reviewboard.mozilla.org/r/254536/#review261746

::: devtools/client/locales/en-US/font-inspector.properties:69
(Diff revision 2)
>  # LOCALIZATION NOTE (fontinspector.familiesNotUsedLabel): Label for the list in the font
>  # editor which contains the names of font families declared but not used.
>  fontinspector.familiesNotUsedLabel=Unused font families
> +
> +# LOCALIZATION NOTE (fontinspector.showMore): Label for a collapsed list of fonts.
> +fontinspector.seeMore=See more

The standard is Show more/less. Is there a specific reason to use "See" (space constraints?)

Comment 23

[Razvan Caliman [:rcaliman]]

Thanks for catching this. Followed-up with a fix in Bug 1473496

> The standard is Show more/less. Is there a specific reason to use "See" (space constraints?)

No particular reason. Changed in the patch to reflect the standard you point out.

Comment 24

[Victoria Wang [:victoria]]

Filed a new bug for further font panel tweaks https://bugzilla.mozilla.org/show_bug.cgi?id=1474731 :)