Closed Bug 1449885 Opened 2 years ago Closed 2 years ago

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

Categories

(DevTools :: Inspector, enhancement)

enhancement
Not set
normal

Tracking

(firefox61 fixed)

RESOLVED FIXED
Firefox 61
Tracking Status
firefox61 --- fixed

People

(Reporter: rcaliman, Assigned: rcaliman)

References

Details

Attachments

(2 files)

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
Blocks: 1449893
Blocks: 1450695
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 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+
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)
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
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.