Closed Bug 1449893 Opened 6 years ago Closed 6 years ago

Font editor: implement UI for variable font instances

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, 2 obsolete files)

If a variable font has any defined instances (aka presets for values of its axes, like Light, Bold, Extra Bold, etc) show a UI that allows selecting one and update the axis values. When a user makes a change to one of the axes, unselect the defined instance and select the default option "Custom".

UI:
https://mozilla.invisionapp.com/share/Z3F7OGCTK#/screens/279202133
Attachment #8968573 - Flags: review?(gl)
Attachment #8968574 - Flags: review?(gl)
Comment on attachment 8968574 [details]
Bug 1449893 - Implementation for managing variation instances. f=gl

https://reviewboard.mozilla.org/r/237270/#review243854

::: devtools/client/inspector/fonts/components/FontEditor.js:93
(Diff revision 1)
> +   *        }
> +   * @return {DOMNode}
> +   */
> +  renderInstances(fontInstances = [], selectedInstance) {
> +    // Append a "Custom" instance entry which represents the latest manual axes changes.
> +    const customInstance = { name: "Custom", values: this.props.fontEditor.snapshot };

"Custom" needs to be localized

::: devtools/client/inspector/fonts/components/FontEditor.js:119
(Diff revision 1)
> +        }
> +      },
> +      instanceOptions
> +    );
> +
> +    // Wrap it up.

Remove this comment, seems unnecessary

::: devtools/client/inspector/fonts/fonts.js:24
(Diff revision 1)
>  const INSPECTOR_L10N =
>    new LocalizationHelper("devtools/client/locales/inspector.properties");
>  
>  const { updateFonts } = require("./actions/fonts");
>  const { updatePreviewText } = require("./actions/font-options");
> -const { resetFontEditor, toggleFontEditor, updateAxis, updateFontEditor } =
> +const {

Move this above font-options

::: devtools/client/inspector/fonts/fonts.js:73
(Diff revision 1)
>      if (!this.inspector) {
>        return;
>      }
>  
>      let fontsApp = FontsApp({
>        onPreviewFonts: this.onPreviewFonts,

Move this below onInstanceChange

::: devtools/client/inspector/fonts/fonts.js:226
(Diff revision 1)
>     * @param  {String} value
>     *         Value of the font axis.
>     */
>    onAxisUpdate(tag, value) {
>      this.store.dispatch(updateAxis(tag, value));
> +    this.store.dispatch(setInstance("Custom", null));

"Custom" needs to be localized. Add this to font-inspector.properties.
Attachment #8968574 - Flags: review?(gl) → review+
Comment on attachment 8968573 [details]
Bug 1449893 - Redux setup for managing font variation instances. f=gl

https://reviewboard.mozilla.org/r/237268/#review243852

::: devtools/client/inspector/fonts/actions/font-editor.js:8
(Diff revision 1)
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
>  "use strict";
>  
>  const {
> +  CREATE_SNAPSHOT,

CREATE_CUSTOM_INSTANCE

::: devtools/client/inspector/fonts/actions/font-editor.js:24
(Diff revision 1)
>      return {
>        type: RESET_EDITOR,
>      };
>    },
>  
> +  setInstance(name, values) {

Rename to updateFontVariationInstance or updateInstance

::: devtools/client/inspector/fonts/actions/font-editor.js:32
(Diff revision 1)
> +      name,
> +      values,
> +    };
> +  },
> +
> +  createSnapshot() {

Move this above resetFontEditor

::: devtools/client/inspector/fonts/actions/font-editor.js:32
(Diff revision 1)
> +      name,
> +      values,
> +    };
> +  },
> +
> +  createSnapshot() {

Rename this to updateCustomInstance.

::: devtools/client/inspector/fonts/actions/index.js:11
(Diff revision 1)
>  
>  const { createEnum } = require("devtools/client/shared/enum");
>  
>  createEnum([
>  
> +  // Create a copy of current axes values.

Update the current axes values for the custom font variation instance.

::: devtools/client/inspector/fonts/reducers/font-editor.js:10
(Diff revision 1)
>  "use strict";
>  
>  const {
> +  CREATE_SNAPSHOT,
>    RESET_EDITOR,
> +  SET_INSTANCE,

s/SET_INSTANCE/UPDATE_FONT_VARIATION_INSTANCE

::: devtools/client/inspector/fonts/reducers/font-editor.js:10
(Diff revision 1)
>  "use strict";
>  
>  const {
> +  CREATE_SNAPSHOT,
>    RESET_EDITOR,
> +  SET_INSTANCE,

Move it be below UPDATE_EDITOR_VISIBILITY

::: devtools/client/inspector/fonts/reducers/font-editor.js:23
(Diff revision 1)
>    axes: {},
>    // Fonts applicable to selected element.
>    fonts: [],
>    // Whether or not the font editor is visible.
>    isVisible: false,
> +  // Currently selected font variation instance.

s/Currently/Current

::: devtools/client/inspector/fonts/reducers/font-editor.js:24
(Diff revision 1)
>    // Fonts applicable to selected element.
>    fonts: [],
>    // Whether or not the font editor is visible.
>    isVisible: false,
> +  // Currently selected font variation instance.
> +  instance: {

Move this above isVisible

::: devtools/client/inspector/fonts/reducers/font-editor.js:24
(Diff revision 1)
>    // Fonts applicable to selected element.
>    fonts: [],
>    // Whether or not the font editor is visible.
>    isVisible: false,
> +  // Currently selected font variation instance.
> +  instance: {

Optionally rename this to be fontVariationInstance so it's a bit more clear

::: devtools/client/inspector/fonts/reducers/font-editor.js:25
(Diff revision 1)
>    fonts: [],
>    // Whether or not the font editor is visible.
>    isVisible: false,
> +  // Currently selected font variation instance.
> +  instance: {
> +    name: "Custom",

"Custom" needs to be localize as well. See font-inspector.properties

::: devtools/client/inspector/fonts/reducers/font-editor.js:29
(Diff revision 1)
> +  instance: {
> +    name: "Custom",
> +    values: [],
> +  },
> +  // Copy of the most recent axes values. Used to revert from a named instance.
> +  snapshot: [],

Move this below selector.

::: devtools/client/inspector/fonts/types.js:92
(Diff revision 1)
>    fonts: PropTypes.arrayOf(PropTypes.shape(font)),
>  
>    // Whether or not the font editor is visible
>    isVisible: PropTypes.bool,
>  
> +  // Variation instance currently selected

s/Variation/Font variation
Attachment #8968573 - Flags: review?(gl) → review+
Comment on attachment 8968574 [details]
Bug 1449893 - Implementation for managing variation instances. f=gl

https://reviewboard.mozilla.org/r/237270/#review243892

::: devtools/client/inspector/fonts/components/FontEditor.js:100
(Diff revision 1)
> +
> +    // Generate the <option> elements for the dropdown.
> +    const instanceOptions = fontInstances.map(instance =>
> +      dom.option(
> +        {
> +          "value": instance.name,

This can just be value and selected without the quotations.
Comment on attachment 8968574 [details]
Bug 1449893 - Implementation for managing variation instances. f=gl

https://reviewboard.mozilla.org/r/237270/#review243896

::: commit-message-263b5:1
(Diff revision 1)
> +Bug 1449893 - Implementation for managing variation instances. f=gl

This needs to become r=gl
Comment on attachment 8968573 [details]
Bug 1449893 - Redux setup for managing font variation instances. f=gl

https://reviewboard.mozilla.org/r/237268/#review243898

::: commit-message-ee1d1:1
(Diff revision 1)
> +Bug 1449893 - Redux setup for managing font variation instances. f=gl

r=gl
Comment on attachment 8968574 [details]
Bug 1449893 - Implementation for managing variation instances. f=gl

https://reviewboard.mozilla.org/r/237270/#review244162
Forwarding r+ from previous patch.
Attachment #8968573 - Attachment is obsolete: true
Attachment #8969615 - Flags: review+
Forwarding r+ from previous patch.
Attachment #8968574 - Attachment is obsolete: true
Attachment #8969616 - Flags: review+
Comment on attachment 8968573 [details]
Bug 1449893 - Redux setup for managing font variation instances. f=gl

https://reviewboard.mozilla.org/r/237268/#review243852

> s/SET_INSTANCE/UPDATE_FONT_VARIATION_INSTANCE

Neither SET_INSTANCE nor UPDATE_FONT_VARIATION_INSTANCE reflect the behaviour of the action. An instance's values are *applied* and change the existing variation axes. The action doesn't do anything *to* the instance, it does something *with* it.

I chose to rename SET_INSTANCE to APPLY_FONT_VARIATION_INSTANCE and update the respective action method from setInstance() to applyInstance().

> Optionally rename this to be fontVariationInstance so it's a bit more clear

The use of "instance" is documented as font variation instance across the methods it is used. I prefer to keep it shorthand to maintain code readability.

> Move this below selector.

Renamed it to customInstanceValues so it clearly reflects what is is.
Keywords: checkin-needed
Pushed by csabou@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/df38004081b3
Redux setup for managing font variation instances. r=gl
https://hg.mozilla.org/integration/mozilla-inbound/rev/07bfb2403ba5
Implementation for managing variation instances. r=gl
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/df38004081b3
https://hg.mozilla.org/mozilla-central/rev/07bfb2403ba5
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 61
Product: Firefox → DevTools
Component: Inspector: Fonts → Inspector
You need to log in before you can comment on or make changes to this bug.