1450695 - Font editor: implement UI for non-variable font

Status: RESOLVED FIXED

(DevTools :: Inspector, enhancement, P3)

Description

[Razvan Caliman [:rcaliman]]

Implement UI controls in font editor for fonts which do not have variable font features.

Designs here: https://mozilla.invisionapp.com/share/Z3F7OGCTK#/screens/279202133

Comment 1

[Razvan Caliman [:rcaliman]]

Attachment: Bug 1450695: Generalize FontAxis into FontPropertyValue for reuse with non-variable fonts.

- Add Redux setup for storing CSS font property values
- Refactor FontAxis component into generalized FontPropertyValue to
support values with units.
- Generalized CSS selectors to apply to UI other than for font axes

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

Comment 2

[Razvan Caliman [:rcaliman]]

Attachment: Bug 1450695: Add support for editing CSS font properties for non-variable fonts.

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

Comment 3

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

Comment on attachment 8974410 [details]
Bug 1450695: Generalize FontAxis into FontPropertyValue for reuse with non-variable fonts.

https://reviewboard.mozilla.org/r/242754/#review249870

::: devtools/client/inspector/fonts/components/FontEditor.js:53
(Diff revision 1)
>  
>      return step.toString();
>    }
>  
>    /**
> -   * Get an array of FontAxis components for of the given variable font axis instances.
> +   * Get an array of UI components for of the given variable font axes.

s/UI/FontPropertyValue

Comment 4

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

Comment on attachment 8974411 [details]
Bug 1450695: Add support for editing CSS font properties for non-variable fonts.

https://reviewboard.mozilla.org/r/242756/#review249872

::: devtools/client/inspector/fonts/components/FontEditor.js:203
(Diff revision 1)
>          className: "theme-sidebar inspector-tabpanel",
>          id: "sidebar-panel-fontinspector"
>        },
> +      // Always render UI for font family, format and font file URL.
>        this.renderFontFamily(font),
> -      fontInstances && this.renderInstances(fontInstances, instance),
> +      // Render UI for font vriation instances if they are defined.

s/vriation/variation

::: devtools/client/inspector/fonts/components/FontEditor.js:204
(Diff revision 1)
>          id: "sidebar-panel-fontinspector"
>        },
> +      // Always render UI for font family, format and font file URL.
>        this.renderFontFamily(font),
> -      fontInstances && this.renderInstances(fontInstances, instance),
> -      fontAxes ?
> +      // Render UI for font vriation instances if they are defined.
> +      hasFontInstances && this.renderInstances(font.variationInstances, instance),

It might be cleaner to have these hasX checks inside of renderX, and then we would cleanly see the order of what renders after another in the main render().

::: devtools/client/inspector/fonts/components/FontStyle.js:22
(Diff revision 1)
> +      value: PropTypes.string.isRequired,
> +    };
> +  }
> +
> +  constructor() {
> +    super();

constructor(props) {
 super(props)
 
 is needed

Comment 5

[Razvan Caliman [:rcaliman]]

Comment on attachment 8974411 [details]
Bug 1450695: Add support for editing CSS font properties for non-variable fonts.

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

Comment 6

[Razvan Caliman [:rcaliman]]

Comment on attachment 8974411 [details]
Bug 1450695: Add support for editing CSS font properties for non-variable fonts.

https://reviewboard.mozilla.org/r/242756/#review249872

> It might be cleaner to have these hasX checks inside of renderX, and then we would cleanly see the order of what renders after another in the main render().

The boolean checks here are intentional. 

From my understanding of this article about optimizing for React, https://evilmartians.com/chronicles/optimizing-react-virtual-dom-explained, (search for "short circuit boolean evaluation"), is that the use of primitive types like boolean is encouraged when the goal is to maintain the component's children array length consistent across re-renders. This prevents unncessary re-rendering and diff-ing of unrelated components.

This pattern is commoon in other parts of DevTools, such as the netmonitor: https://searchfox.org/mozilla-central/source/devtools/client/netmonitor/src/components/RequestListItem.js#207

I agree readability is not ideal, but I'd like to keep this pattern.

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/2e4c21fb64ae
Generalize FontAxis into FontPropertyValue for reuse with non-variable fonts. r=gl
https://hg.mozilla.org/integration/autoland/rev/a8bad1f53a34
Add support for editing CSS font properties for non-variable fonts. r=gl

Comment 8

[Dorel Luca [:dluca]]

https://hg.mozilla.org/mozilla-central/rev/2e4c21fb64ae
https://hg.mozilla.org/mozilla-central/rev/a8bad1f53a34