1449885 - Font editor: implement UI to manage font-variation-settings

Status: RESOLVED FIXED

(DevTools :: Inspector, enhancement)

Description

[Razvan Caliman [:rcaliman]]

Implement a UI which allows editing of individual variable font axis values and that writes the changes back to the CSS rule where editing originated.

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

Comment 1

[Razvan Caliman [:rcaliman]]

Attachment: Bug 1449885 - Read font variation axis data and setup UI with any axis values defined on rule.

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

Comment 2

[Razvan Caliman [:rcaliman]]

Attachment: Bug 1449885 - Write variation axis changes to rule and keep in sync with any manual edits to rule.

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

Comment 3

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

Comment on attachment 8964299 [details]
Bug 1449885 - Read font variation axis data and setup UI with any axis values defined on rule.

https://reviewboard.mozilla.org/r/233010/#review239956

::: devtools/client/inspector/fonts/components/FontEditor.js:9
(Diff revision 1)
>  
>  "use strict";
>  
> -const { PureComponent } = require("devtools/client/shared/vendor/react");
> +const { createFactory, PureComponent } = require("devtools/client/shared/vendor/react");
>  const dom = require("devtools/client/shared/vendor/react-dom-factories");
> +const FontAxis = createFactory(require("./FontAxis"));

Move this below PropTypes

::: devtools/client/inspector/fonts/components/FontEditor.js:9
(Diff revision 1)
>  
>  "use strict";
>  
> -const { PureComponent } = require("devtools/client/shared/vendor/react");
> +const { createFactory, PureComponent } = require("devtools/client/shared/vendor/react");
>  const dom = require("devtools/client/shared/vendor/react-dom-factories");
> +const FontAxis = createFactory(require("./FontAxis"));

Move this below PropTypes. Looking at other React components we have this convention going where we keep the full path requires at the top, follow by createFactory and requires that have relative paths.

These are usually spaced out by a new line that you also consequently removed before Types.

::: devtools/client/inspector/fonts/components/FontEditor.js:24
(Diff revision 1)
>    }
>  
> +  /**
> +   * Naive implementation to get increment step for variable font axis that ensures
> +   * a wide spectrum of precision based on range of values between min and max.
> +   * @param {Number|String} min

Add a new line between the description and @param as you have done for your other JSdoc

::: devtools/client/inspector/fonts/fonts.js:164
(Diff revision 1)
>    /**
> +   * Handler for changes of font axis value. Updates the value in the store and previews
> +   * the change on the page.
> +   *
> +   * @param {String} tag
> +   *        Name (aka tag) of the font axis.

Tag name of the font axis.

::: devtools/client/inspector/fonts/fonts.js:251
(Diff revision 1)
> +   * values change in the Rule view. For the latter case, we do a deep compare between the
> +   * font properties on the selected rule and the ones already store to decide if to
> +   * update the font edtior to reflect a new external state.
> +   */
> +  async refreshFontEditor() {
> +    if (!this.selectedRule) {

We don't have unit tests for this yet, but we will probably need to do it soon.

I would imagine you will need a check for this.inspector and this.store once you start running unit tests.

::: devtools/client/inspector/fonts/fonts.js:271
(Diff revision 1)
> +      }
> +
> +      return acc;
> +    }, {});
> +
> +    const state = this.store.getState().fontEditor;

s/state/fontEditor

::: devtools/client/themes/fonts.css:120
(Diff revision 1)
> +  justify-content: space-between;
> +  align-items: center;
> +  padding: 5px 20px;
> +}
> +
> +.font-axis__label {

I would prefer us to stick to our typical css naming convention which would mean we can't use __ and instead do .font-axis-label, .font-axis-slider, etc

Comment 4

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

Comment on attachment 8964300 [details]
Bug 1449885 - Write variation axis changes to rule and keep in sync with any manual edits to rule.

https://reviewboard.mozilla.org/r/233012/#review239958

::: devtools/client/inspector/fonts/fonts.js:167
(Diff revision 1)
>    /**
> +   * Live preview all CSS font property values from the fontEditor store on the page
> +   * and sync the changes to the Rule view.
> +   */
> +  applyChanges() {
> +    const state = this.store.getState().fontEditor;

s/state/fontEditor

::: devtools/client/inspector/fonts/fonts.js:201
(Diff revision 1)
> +   *        Model of CSS declaration for a property in used in the rule view.
> +   * @param {String} value
> +   *        Value of the CSS property that should be reflected in the rule view.
> +   */
> +  syncChanges(textProperty, value) {
> +    textProperty.noticeNewValue(value);

noticeNewValue seems like an odd function name. I would prefer if this was to be something like updateValue.

Comment 5

[Razvan Caliman [:rcaliman]]

Comment on attachment 8964299 [details]
Bug 1449885 - Read font variation axis data and setup UI with any axis values defined on rule.

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

Comment 6

[Razvan Caliman [:rcaliman]]

Comment on attachment 8964300 [details]
Bug 1449885 - Write variation axis changes to rule and keep in sync with any manual edits to rule.

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

Comment 7

[Razvan Caliman [:rcaliman]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=37f488cd63830eb2f933f5bb54187b38c0b716cf

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7ee122acafd1
Read font variation axis data and setup UI with any axis values defined on rule. r=gl
https://hg.mozilla.org/integration/mozilla-inbound/rev/47b3480dc0af
Write variation axis changes to rule and keep in sync with any manual edits to rule. r=gl

Comment 9

[Arthur Iakab [arthur_iakab]]

Backed out 2 changesets (bug 1449885) For frequently failing devtools on browser_parsable_css.js on a CLOSED TREE
Link to the log: https://treeherder.mozilla.org/logviewer.html#?job_id=172914887&repo=mozilla-inbound&lineNumber=2140
Backout revision https://hg.mozilla.org/integration/mozilla-inbound/rev/b324b445ca11a50859161c2a787fc74ce4606f2f
Failed push https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=47b3480dc0afb7fed257fe4963e4738957e381d9&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=pending&filter-resultStatus=running&filter-classifiedState=unclassified&filter-searchStr=devtools

Comment 10

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

This is because we wrote flex-direction: row nowrap;

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3f00b6633b3b
Read font variation axis data and setup UI with any axis values defined on rule. r=gl
https://hg.mozilla.org/integration/mozilla-inbound/rev/09c70b07e2e4
Write variation axis changes to rule and keep in sync with any manual edits to rule. r=gl

Comment 12

[Cristina Coroiu [:ccoroiu]]

https://hg.mozilla.org/mozilla-central/rev/3f00b6633b3b
https://hg.mozilla.org/mozilla-central/rev/09c70b07e2e4