Closed Bug 1472966 Opened 6 years ago Closed 6 years ago

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

Categories

(DevTools :: Inspector, enhancement, P3)

Product:
DevTools
Component:
Inspector
Type:
enhancement

Tracking

(firefox63 fixed)

Status:
RESOLVED FIXED
Milestone:
Firefox 63
Tracking Flags:
Tracking Status
firefox63 --- fixed

People

(Reporter: rcaliman, Assigned: rcaliman)

References

Details

Attachments

(6 files)

Bug 1472966 - Styling tweaks for Font Editor.
59 bytes, text/x-review-board-request
gl
: review+
Details
Bug 1472966 - Replace label for list of unused font families.
59 bytes, text/x-review-board-request
gl
: review+
Details
Bug 1472966 - Limit list of used fonts to 3 and put others in a collapsed list.
59 bytes, text/x-review-board-request
gl
: review+
Details
Bug 1472966 - Do not show "Rendered fonts" accordion if there are no fonts to show.
59 bytes, text/x-review-board-request
gl
: review+
Details
hide-font-name-if-equals-family-name.mp4
183.57 KB, video/mp4
Details
Bug 1472966 - Miscellaneous tweaks to paddings, margins and borders for font editor.
59 bytes, text/x-review-board-request
gl
: review+
Details 
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
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 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
Attachment #8989503 - Flags: review?(gl) → review+
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
Attachment #8989502 - Flags: review?(gl) → review+
Comment on attachment 8989501 [details]
Bug 1472966 - Replace label for list of unused font families.

https://reviewboard.mozilla.org/r/254534/#review261468
Attachment #8989501 - Flags: review?(gl) → review+
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 "/"
Attachment #8989500 - Flags: review?(gl) → review+
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 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
Attachment #8989723 - Flags: review?(gl) → review+
Pushed by rcaliman@mozilla.com:
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 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.
Flags: needinfo?(rcaliman)
Flags: needinfo?(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/#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?)
See Also: → 1473496
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.
Flags: needinfo?(rcaliman)
Flags: needinfo?(gl)
Filed a new bug for further font panel tweaks https://bugzilla.mozilla.org/show_bug.cgi?id=1474731 :)
QA Contact: catalin.sasca
Component: Inspector: Fonts → Inspector
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: