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

RESOLVED FIXED in Firefox 61

Status

RESOLVED FIXED
9 months ago
6 months ago

People

(Reporter: rcaliman, Assigned: rcaliman)

Tracking

(Blocks: 1 bug)

unspecified
Firefox 61
Dependency tree / graph

Firefox Tracking Flags

(firefox61 fixed)

Details

Attachments

(2 attachments)

(Assignee)

Description

9 months ago
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
(Assignee)

Updated

9 months ago
Blocks: 1449893
(Assignee)

Updated

8 months ago
Blocks: 1450695
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 3

8 months ago
mozreview-review
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
Attachment #8964299 - Flags: review?(gl) → review+

Comment 4

8 months ago
mozreview-review
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.
Attachment #8964300 - Flags: review?(gl) → review+
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 8

8 months ago
Pushed by gabriel.luong@gmail.com:
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
This is because we wrote flex-direction: row nowrap;
Flags: needinfo?(rcaliman)

Comment 11

8 months ago
Pushed by gabriel.luong@gmail.com:
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

8 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/3f00b6633b3b
https://hg.mozilla.org/mozilla-central/rev/09c70b07e2e4
Status: NEW → RESOLVED
Last Resolved: 8 months ago
status-firefox61: --- → fixed
Resolution: --- → FIXED

Updated

6 months ago
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.