Closed
Bug 1449893
Opened 6 years ago
Closed 6 years ago
Font editor: implement UI for variable font instances
Categories
(DevTools :: Inspector, enhancement)
DevTools
Inspector
Tracking
(firefox61 fixed)
RESOLVED
FIXED
Firefox 61
Tracking | Status | |
---|---|---|
firefox61 | --- | fixed |
People
(Reporter: rcaliman, Assigned: rcaliman)
References
Details
Attachments
(2 files, 2 obsolete files)
7.96 KB,
patch
|
rcaliman
:
review+
|
Details | Diff | Splinter Review |
13.05 KB,
patch
|
rcaliman
:
review+
|
Details | Diff | Splinter Review |
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
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Updated•6 years ago
|
Attachment #8968573 -
Flags: review?(gl)
Attachment #8968574 -
Flags: review?(gl)
Comment 3•6 years ago
|
||
mozreview-review |
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 4•6 years ago
|
||
mozreview-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 5•6 years ago
|
||
mozreview-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 6•6 years ago
|
||
mozreview-review |
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 7•6 years ago
|
||
mozreview-review |
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
Assignee | ||
Comment 8•6 years ago
|
||
mozreview-review |
Comment on attachment 8968574 [details] Bug 1449893 - Implementation for managing variation instances. f=gl https://reviewboard.mozilla.org/r/237270/#review244162
Assignee | ||
Comment 9•6 years ago
|
||
Forwarding r+ from previous patch.
Attachment #8968573 -
Attachment is obsolete: true
Attachment #8969615 -
Flags: review+
Assignee | ||
Comment 10•6 years ago
|
||
Forwarding r+ from previous patch.
Attachment #8968574 -
Attachment is obsolete: true
Attachment #8969616 -
Flags: review+
Assignee | ||
Comment 11•6 years ago
|
||
mozreview-review-reply |
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.
Assignee | ||
Updated•6 years ago
|
Keywords: checkin-needed
Comment 12•6 years ago
|
||
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
Comment 13•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/df38004081b3 https://hg.mozilla.org/mozilla-central/rev/07bfb2403ba5
Status: NEW → RESOLVED
Closed: 6 years ago
status-firefox61:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 61
Updated•6 years ago
|
Product: Firefox → DevTools
Updated•3 years ago
|
Component: Inspector: Fonts → Inspector
You need to log in
before you can comment on or make changes to this bug.
Description
•