Closed Bug 1443846 Opened 6 years ago Closed 6 years ago

Implement toggle in Rule view for font editor panel

Categories

(DevTools :: Inspector: Rules, enhancement, P3)

Product:
DevTools
Component:
Inspector: Rules
Type:
enhancement

Tracking

(firefox61 fixed)

Status:
RESOLVED FIXED
Milestone:
Firefox 61
Tracking Flags:
Tracking Status
firefox61 --- fixed

People

(Reporter: rcaliman, Assigned: rcaliman)

References

Details

Attachments

(3 files)

Bug 1443846 - Add swatch to mark rule as selected and toggle font inspector panel.
59 bytes, text/x-review-board-request
pbro
: review+
Details
Bug 1443846 - Add skeleton for font editor panel.
59 bytes, text/x-review-board-request
gl
: review+
Details
Bug 1443846 - Add SVG swatch icon for font editor toggle.
59 bytes, text/x-review-board-request
pbro
: review+
Details 
Implement toggle button in Rule view which triggers an empty panel to be used by the variable fonts editor. While on, this panel replaces the fonts overview panel.

Design here:
https://mozilla.invisionapp.com/share/TUE5VV8MV#/screens/279202133
Component: Developer Tools: Inspector → Developer Tools: CSS Rules Inspector
Priority: -- → P3
Comment on attachment 8958606 [details]
Bug 1443846 - Add skeleton for font editor panel.

https://reviewboard.mozilla.org/r/227516/#review233756

::: devtools/client/inspector/fonts/components/FontEditor.js:17
(Diff revision 1)
> +const TOOL_ID = "fonteditor";
> +
> +class FontEditor extends PureComponent {
> +  static get propTypes() {
> +    return {
> +      ruleView: PropTypes.instanceOf(CssRuleView).isRequired

We should never pass the rule view object as a prop into the React component. The FontInspector object should really be acting like a controller for these events and sending an action to toggle this FontEditor state by changing the Redux state for the fonts.

::: devtools/client/inspector/fonts/fonts.js:15
(Diff revision 1)
>  const { getColor } = require("devtools/client/shared/theme");
> -const { createFactory, createElement } = require("devtools/client/shared/vendor/react");
> +const { createFactory, createElement, Fragment } = require("devtools/client/shared/vendor/react");
>  const { Provider } = require("devtools/client/shared/vendor/react-redux");
>  
>  const FontsApp = createFactory(require("./components/FontsApp"));
> +const FontEditor = createFactory(require("./components/FontEditor"));

I think this should be a component inside of FontsApp.
Attachment #8958606 - Flags: review?(gl)
Comment on attachment 8958605 [details]
Bug 1443846 - Add swatch to mark rule as selected and toggle font inspector panel.

https://reviewboard.mozilla.org/r/227514/#review233760

::: devtools/client/inspector/rules/rules.js:109
(Diff revision 1)
>    this.inspector = inspector;
>    this.highlighters = inspector.highlighters;
>    this.styleDocument = document;
>    this.styleWindow = this.styleDocument.defaultView;
>    this.store = store || {};
> +  this.selectedRules = {};

Maybe use a Map object.

::: devtools/client/inspector/rules/rules.js:109
(Diff revision 1)
>    this.inspector = inspector;
>    this.highlighters = inspector.highlighters;
>    this.styleDocument = document;
>    this.styleWindow = this.styleDocument.defaultView;
>    this.store = store || {};
> +  this.selectedRules = {};

Also, add a comment about what this.selectedRules represent.

::: devtools/client/inspector/rules/rules.js:170
(Diff revision 1)
> +    this._handleFontEditorPrefChange.bind(this);
>  
>    this._prefObserver = new PrefObserver("devtools.");
>    this._prefObserver.on(PREF_UA_STYLES, this._handleUAStylePrefChange);
>    this._prefObserver.on(PREF_DEFAULT_COLOR_UNIT, this._handleDefaultColorUnitPrefChange);
> +  this._prefObserver.on(PREF_FONT_EDITOR, this._handleFontEditorPrefChange);

I don't think we need a pref observer for this pref. This is only useful if the content you are looking at in the rules view needs to be updated because we are toggling the pref from a setting/gear dropdown. Since we know we would be hiding this feature behind the pref and later enabling it, it would actually be work that we need to remove later.

::: devtools/client/inspector/rules/rules.js:173
(Diff revision 1)
>    this._prefObserver.on(PREF_UA_STYLES, this._handleUAStylePrefChange);
>    this._prefObserver.on(PREF_DEFAULT_COLOR_UNIT, this._handleDefaultColorUnitPrefChange);
> +  this._prefObserver.on(PREF_FONT_EDITOR, this._handleFontEditorPrefChange);
>  
>    this.showUserAgentStyles = Services.prefs.getBoolPref(PREF_UA_STYLES);
> +  this.enableFontEditor = Services.prefs.getBoolPref(PREF_FONT_EDITOR);

s/enableFontEditor/showFontEditor

::: devtools/client/inspector/rules/rules.js:1219
(Diff revision 1)
>    /**
> +  * Mark a rule as selected for the given toolId.
> +  *
> +  * Editing tools can mark one or more rules as selected for themselves so they have
> +  * a reference of where to make changes, like add / remove properties.
> +  * Each tool has an identififer string (aka toolId) which is used as a key in a map that

s/identififer/identifier

::: devtools/client/inspector/rules/rules.js:1221
(Diff revision 1)
> +  *
> +  * Editing tools can mark one or more rules as selected for themselves so they have
> +  * a reference of where to make changes, like add / remove properties.
> +  * Each tool has an identififer string (aka toolId) which is used as a key in a map that
> +  * holds references to Rule objects.
> +  * Many tools may operate at the same time (ex: Font Editor and Shape Editor) so there

s/Shape/Shape Path

::: devtools/client/inspector/rules/rules.js:1225
(Diff revision 1)
> +  * holds references to Rule objects.
> +  * Many tools may operate at the same time (ex: Font Editor and Shape Editor) so there
> +  * are multiple possible selected rules at any given time. A rule can be selected by
> +  * different tools at the same time, with each tool operating independently on it.
> +  *
> +  * @param {Rule}    rule

The @param indentation doesn't quite conform here. Only one space after the "}" and the next line should start from "{"

::: devtools/client/inspector/rules/rules.js:1227
(Diff revision 1)
> +  * are multiple possible selected rules at any given time. A rule can be selected by
> +  * different tools at the same time, with each tool operating independently on it.
> +  *
> +  * @param {Rule}    rule
> +  *                  Rule object for which to hold a reference.
> +  * @param {String}  toolId

toolId might be confusing in this context because we use toolId to describe each panel as an individual tool if you do a search for "toolId". I would be tempted to rename this to editorId or id.

::: devtools/client/inspector/rules/rules.js:1234
(Diff revision 1)
> +  * @param {Boolean} [unselectOthers=true]
> +  *                  Optional. Default: `true`. If true, unselect all other rules that
> +  *                  were selected for the given tool. Ensures only one rule at a time is
> +  *                  selected for a particular tool. Set to `false` if a tool may operate
> +  *                  on multiple rules at a time.
> +  *

No new line before @return

::: devtools/client/inspector/rules/rules.js:1241
(Diff revision 1)
> +  */
> +  selectRule(rule, toolId, unselectOthers = true) {
> +    if (!this.selectedRules[toolId]) {
> +      this.selectedRules[toolId] = [];
> +    }
> +    if (!this.selectedRules[toolId].includes(rule)) {

Add a new line before this to separate the if statement blocks.

::: devtools/client/inspector/rules/rules.js:1241
(Diff revision 1)
> +  */
> +  selectRule(rule, toolId, unselectOthers = true) {
> +    if (!this.selectedRules[toolId]) {
> +      this.selectedRules[toolId] = [];
> +    }
> +    if (!this.selectedRules[toolId].includes(rule)) {

Actually this should be an else if

::: devtools/client/inspector/rules/rules.js:1243
(Diff revision 1)
> +    if (!this.selectedRules[toolId]) {
> +      this.selectedRules[toolId] = [];
> +    }
> +    if (!this.selectedRules[toolId].includes(rule)) {
> +      this.selectedRules[toolId].push(rule);
> +      if (unselectOthers) {

Same as above about adding a new line before the if statement

::: devtools/client/inspector/rules/rules.js:1244
(Diff revision 1)
> +      this.selectedRules[toolId] = [];
> +    }
> +    if (!this.selectedRules[toolId].includes(rule)) {
> +      this.selectedRules[toolId].push(rule);
> +      if (unselectOthers) {
> +        this.selectedRules[toolId]

This can probably use a comment to quickly understand that rules are being unselected.

::: devtools/client/inspector/rules/rules.js:1245
(Diff revision 1)
> +    }
> +    if (!this.selectedRules[toolId].includes(rule)) {
> +      this.selectedRules[toolId].push(rule);
> +      if (unselectOthers) {
> +        this.selectedRules[toolId]
> +          .filter(item => (item !== rule))

We can remove the ( ) around item !== rule

::: devtools/client/inspector/rules/rules.js:1251
(Diff revision 1)
> +          .map(item => this.unselectRule(item, toolId));
> +      }
> +    }
> +
> +    this.emit("ruleview-rule-selected", {toolId, rule});
> +    return rule;

I don't see the return rule being used. Should we really return the rule?

::: devtools/client/inspector/rules/rules.js:1265
(Diff revision 1)
> +   *                 Key for which to mark the given rule as selected.
> +   */
> +  unselectRule(rule, toolId) {
> +    if (!Array.isArray(this.selectedRules[toolId])) {
> +      return;
> +    }

Add a new line after this if statement block.

::: devtools/client/inspector/rules/rules.js:1271
(Diff revision 1)
> +    let index = this.selectedRules[toolId].findIndex((item) => {
> +      return item === rule;
> +    });
> +    if (index === -1) {
> +      return;
> +    }

Same as above

::: devtools/client/inspector/rules/rules.js:1286
(Diff revision 1)
> +  *                 Optional tool ID for which to restrict unselect operation.
> +  */
> +  unselectAllRules(toolId) {
> +    let keys = Object.keys(this.selectedRules);
> +    keys = toolId ? keys.filter(key => (key === toolId)) : keys;
> +    keys.forEach(key => {

Use "for (let key of keys) { }" instead of forEach.

::: devtools/client/inspector/rules/views/rule-editor.js:92
(Diff revision 1)
>  
>  RuleEditor.prototype = {
>    destroy: function() {
>      this.rule.domRule.off("location-changed");
>      this.toolbox.off("tool-registered", this._onToolChanged);
>      this.toolbox.off("tool-unregistered", this._onToolChanged);

You need to .off() the events as well
Attachment #8958605 - Flags: review?(gl)
Comment on attachment 8958605 [details]
Bug 1443846 - Add swatch to mark rule as selected and toggle font inspector panel.

https://reviewboard.mozilla.org/r/227514/#review233840

Thanks a lot Razvan!

::: devtools/client/inspector/rules/rules.js:170
(Diff revision 1)
> +    this._handleFontEditorPrefChange.bind(this);
>  
>    this._prefObserver = new PrefObserver("devtools.");
>    this._prefObserver.on(PREF_UA_STYLES, this._handleUAStylePrefChange);
>    this._prefObserver.on(PREF_DEFAULT_COLOR_UNIT, this._handleDefaultColorUnitPrefChange);
> +  this._prefObserver.on(PREF_FONT_EDITOR, this._handleFontEditorPrefChange);

I agree with Gabriel. We need the pref, for sure, but we don't necessarily need to observe changes to the pref.
If you just remove this mechanism, then the pref can stay, and it only means that the feature won't be avaible if the pref is off, and one would need to go in about:config, and restart DevTools to see the change, which is more than acceptable.

::: devtools/client/inspector/rules/rules.js:593
(Diff revision 1)
>    _handleDefaultColorUnitPrefChange: function() {
>      this._handlePrefChange(PREF_DEFAULT_COLOR_UNIT);
>    },
>  
> +  _handleFontEditorPrefChange: function() {
> +    this.enableFontEditor = Services.prefs.getBoolPref(PREF_FONT_EDITOR);

You can therefore do this just once at the start and that's it.

::: devtools/client/inspector/rules/views/rule-editor.js:262
(Diff revision 1)
> +  _onRuleSelected: function(eventName, eventData) {
> +    const { rule } = eventData;
> +    const showFontEditor =
> +      this.ruleView.getSelectedRules("fonteditor").includes(rule);
> +    if (this.ruleView.enableFontEditor && showFontEditor) {
> +      this.ruleView.inspector.sidebar.show("fontinspector");
> +    }
> +  },
> +
> +  _onRuleUnselected: function(eventName, eventData) {
> +    const { rule } = eventData;
> +    if (this.ruleView.enableFontEditor && rule == this.rule) {
> +      this.fontSwatch.classList.remove("active");
> +    }
>    },

The rule-editor shouldn't need to worry about anything else than its own logic. 
However here, it both asks for a rule to be selected/unselected *and* switches to the font editor when this happens.
I don't think it should do this second part.

In my mind it should mark a rule as selected/unselected and that's it.

And then another part of the system should listen to the event and switch the right panel. Ideally, this would be a parent of both the rule view and font panel. Because this parent has knowledge of both things, so the code that links them together should live at that level.

In fact, it's a bit awkward here to have to filter on this.rule === rule, but that would become irrelevant if you moved this code up the hierarchy of component.
Attachment #8958605 - Flags: review?(pbrosset)
Comment on attachment 8958606 [details]
Bug 1443846 - Add skeleton for font editor panel.

https://reviewboard.mozilla.org/r/227516/#review233850

I'll let Gabe do the reviews on this one, he's far more knowledgeable than I am on this part.
Attachment #8958606 - Flags: review?(pbrosset)
Just a heads up that some refactoring of event-emitter that happened last week on the inspector has broken patches here:

"TypeError: eventData is undefined: onRuleSelected@resource://devtools/shared/base-loader.js -> resource://devtools/client/inspector/fonts/fonts.js:107:11"

This happened because we're now using a new event-emitter that does not set a first "event" argument in calllbacks.
Sorry if this was going to be addressed in a follow-up bug, but I found out while testing the patches that you can select more than one rule at a time, which I don't think is what we want here, right?
I was able to click on mutliple of the Aa icons, and turn several of them to the selected blue color.
(In reply to Patrick Brosset <:pbro> from comment #11)
> Sorry if this was going to be addressed in a follow-up bug, but I found out
> while testing the patches that you can select more than one rule at a time,
> which I don't think is what we want here, right?
> I was able to click on mutliple of the Aa icons, and turn several of them to
> the selected blue color.
I take that back. After fixing all of the instances of the errors from comment 10, this now works fine.
Comment on attachment 8958605 [details]
Bug 1443846 - Add swatch to mark rule as selected and toggle font inspector panel.

https://reviewboard.mozilla.org/r/227514/#review234540

I really like the separation of concerns now! Thanks.
I have just made a few simple comments, but once addressed, no need to ask for another review from me on this one. It should be ready to go then.

::: devtools/client/inspector/rules/rules.js:1226
(Diff revision 2)
> +  *        selected for the given editor. Ensures only one rule at a time is selected for
> +  *        a particular editor. Set to `false` if an editor may operate on multiple rules
> +  *        at a time.
> +  */
> +  selectRule(rule, editorId, unselectOthers = true) {
> +    this.selectedRules[editorId] = this.selectedRules[editorId] || [];

Maybe call this.getSelectedRules(editorId) here instead since it wraps the logic of initializing an empty array already.

::: devtools/client/inspector/rules/rules.js:1231
(Diff revision 2)
> +      // Mark other rules for this editorId as unselected.
> +      if (unselectOthers) {
> +        this.selectedRules[editorId]
> +          .filter(item => (item !== rule))
> +          .map(item => this.unselectRule(item, editorId));
> +      }

If the rule was already selected, we wouldn't go into the parent if, but we might still want to unselect the other rules. So this should come first, at the top of the function, outside of the parent if.

::: devtools/client/inspector/rules/rules.js:1239
(Diff revision 2)
> +          .filter(item => (item !== rule))
> +          .map(item => this.unselectRule(item, editorId));
> +      }
> +    }
> +
> +    this.onRuleSelected(rule, editorId);

To make this even more generic, you could listen to ruleview-rule-selected events in this very same instance, and use it to toggle the font panel, instead of calling `this.onRuleSelected` manually here.

::: devtools/client/inspector/rules/rules.js:1240
(Diff revision 2)
> +          .map(item => this.unselectRule(item, editorId));
> +      }
> +    }
> +
> +    this.onRuleSelected(rule, editorId);
> +    this.emit("ruleview-rule-selected", {editorId, rule});

Should we move the emit inside the if body above? This way we only emit if we really selected the rule, and not if the rule was already selected before.

::: devtools/client/inspector/rules/rules.js:1256
(Diff revision 2)
> +    let index = this.selectedRules[editorId].findIndex((item) => {
> +      return item === rule;
> +    });

Works nicely as a one liner:

let index = this.selectedRules[editorId].findIndex(item => item === rule);
Attachment #8958605 - Flags: review?(pbrosset) → review+
Attachment #8959580 - Flags: review?(gl) → review?(pbrosset)
Comment on attachment 8959580 [details]
Bug 1443846 - Add SVG swatch icon for font editor toggle.

https://reviewboard.mozilla.org/r/228380/#review234550

Stealing this from Gabriel as this is a simple one I can do quickly.
Looks great to me!
Thanks for using the svg context property trick, this makes it work nicely in all themes.
Attachment #8959580 - Flags: review?(pbrosset) → review+
Comment on attachment 8958605 [details]
Bug 1443846 - Add swatch to mark rule as selected and toggle font inspector panel.

https://reviewboard.mozilla.org/r/227514/#review233760

> I don't think we need a pref observer for this pref. This is only useful if the content you are looking at in the rules view needs to be updated because we are toggling the pref from a setting/gear dropdown. Since we know we would be hiding this feature behind the pref and later enabling it, it would actually be work that we need to remove later.

As a result of this pref observer callback, the rule editor DOM gets rebuilt and the font editor toggle added. I thought this would be a convenience for people to be able to use the toggle immediately after enabling the pref instead of having to refresh DevTools.

> I don't see the return rule being used. Should we really return the rule?

Not used yet. Returned just as a convenience for future uses. This can be removed now if not needed.
Comment on attachment 8958605 [details]
Bug 1443846 - Add swatch to mark rule as selected and toggle font inspector panel.

https://reviewboard.mozilla.org/r/227514/#review235324

::: devtools/client/inspector/rules/rules.js:113
(Diff revision 3)
>    this.styleDocument = document;
>    this.styleWindow = this.styleDocument.defaultView;
>    this.store = store || {};
> +  // References to rules marked by various editors where they intend to write changes.
> +  // @see selectRule(), unselectRule()
> +  this.selectedRules = {};

I think I would still prefer seeing a Map object used here since this is really waht the object represents and it would also provide proper iterators like keys()

::: devtools/client/inspector/rules/rules.js:1271
(Diff revision 3)
> +
> +  /**
> +  * Unmark all selected rules for all editors. If an editor id is provided, unmark all
> +  * selected rules just for that editor leaving others untouched.
> +  *
> +  * @param {String} [editorId]

Remove the []

::: devtools/client/inspector/rules/views/rule-editor.js:253
(Diff revision 3)
> +    if (this.ruleView.showFontEditor) {
> +      this.fontSwatch = createChild(this.element, "div", {
> +        class: "ruleview-fontswatch"
> +      });
> +
> +      // TODO: replace with tool icon and use this as visually hidden a11y text.

Add a Bug Number with associated TODOs
Attachment #8958605 - Flags: review?(gl)
Comment on attachment 8958606 [details]
Bug 1443846 - Add skeleton for font editor panel.

https://reviewboard.mozilla.org/r/227516/#review235326

::: devtools/client/inspector/fonts/actions/font-editor.js:12
(Diff revision 3)
> +const {
> +  SHOW_EDITOR,
> +  HIDE_EDITOR,
> +} = require("./index");
> +
> +module.exports = {

I prefer to sort the list of actions alphabetically. That just means moving hideFontEditor above showFontEditor

::: devtools/client/inspector/fonts/actions/font-editor.js:17
(Diff revision 3)
> +module.exports = {
> +
> +  showFontEditor(selector) {
> +    return {
> +      type: SHOW_EDITOR,
> +      selector

Add trailing commas like you have done in your require. This just makes it easier for the next person who might need to edit these objects and need to add an additional property.

::: devtools/client/inspector/fonts/actions/font-editor.js:23
(Diff revision 3)
> +    };
> +  },
> +
> +  hideFontEditor() {
> +    return {
> +      type: HIDE_EDITOR

Same as above.

::: devtools/client/inspector/fonts/actions/index.js:18
(Diff revision 3)
>  
>    // Update the preview text.
>    "UPDATE_PREVIEW_TEXT",
>  
> +  // Show font editor
> +  "SHOW_EDITOR",

Alphabetically sort ordered these.

::: devtools/client/inspector/fonts/components/FontEditor.js:15
(Diff revision 3)
> +const Types = require("../types");
> +
> +class FontEditor extends PureComponent {
> +  static get propTypes() {
> +    return {
> +      fontEditorData: PropTypes.shape(Types.fontEditorData).isRequired

Add a trailing comma

::: devtools/client/inspector/fonts/components/FontEditor.js:26
(Diff revision 3)
> +
> +    return dom.div(
> +      {
> +        className: "theme-sidebar inspector-tabpanel",
> +        style: {
> +          display: `${isVisible ? "block" : "none"}`

Remove this. See below.

::: devtools/client/inspector/fonts/components/FontOverview.js:20
(Diff revision 3)
> +const Types = require("../types");
> +
> +class FontOverview extends PureComponent {
> +  static get propTypes() {
> +    return {
> +      fontEditorData: PropTypes.shape(Types.fontEditorData).isRequired,

FontOverview shouldn't need to know anything about the FontEditor since they are unrelated. See below.

::: devtools/client/inspector/fonts/components/FontOverview.js:86
(Diff revision 3)
> +
> +    return dom.div(
> +      {
> +        id: "font-container",
> +        style: {
> +          display: `${isVisible ? "block" : "none"}`

Remove this. See below

::: devtools/client/inspector/fonts/components/FontsApp.js:33
(Diff revision 3)
>      return dom.div(
>        {
>          className: "theme-sidebar inspector-tabpanel",
>          id: "sidebar-panel-fontinspector"
>        },
> -      dom.div(
> +      FontEditor(this.props),

Instead of checking isVisible and applying display block or none in FontEditor or FontOverview we can just check if the FontEditor isVisible and render the components as appropriate.

isVisible ?
 FontEditor({})
 :
 FontOverview({})
 
We don't need to pass in the FontEditorData into FontOverview this way and keep the separation of concerns, but it does mean you need to specify the props to pass into each component.

::: devtools/client/inspector/fonts/fonts.js:32
(Diff revision 3)
>    constructor(inspector, window) {
>      this.document = window.document;
>      this.inspector = inspector;
>      this.pageStyle = this.inspector.pageStyle;
> +    this.ruleView = this.inspector.getPanel("ruleview").view;
> +    this.selectedRule = null;

This might be something we want to store in the redux store in the future.

::: devtools/client/inspector/fonts/fonts.js:39
(Diff revision 3)
>      this.update = this.update.bind(this);
> -
>      this.onNewNode = this.onNewNode.bind(this);
>      this.onPreviewFonts = this.onPreviewFonts.bind(this);
>      this.onThemeChanged = this.onThemeChanged.bind(this);
> +    this.onRuleSelected = this.onRuleSelected.bind(this);

Move these below onPreviewFonts

::: devtools/client/inspector/fonts/fonts.js:94
(Diff revision 3)
>        return !nodeFonts.some(nodeFont => nodeFont.name === font.name);
>      });
>    }
>  
>    /**
> +   * Called when a rule from the Rule view was marked as selected for an editor.

Maybe reword this to be

Handler for "ruleview-rule-selected" event emitted from the rule view when a rule is marked as selected for an editor.

::: devtools/client/inspector/fonts/fonts.js:104
(Diff revision 3)
> +   * @param {Object} eventData
> +   *        Data payload for the event. Contains:
> +   *        - {String} editorId - id of the editor for which the rule was selected
> +   *        - {Rule} rule - reference to rule that was selected
> +   */
> +  onRuleSelected(eventData) {

Move these functions below onPreviewFonts() on line 197

::: devtools/client/inspector/fonts/fonts.js:114
(Diff revision 3)
> +      this.store.dispatch(showFontEditor(selector));
> +    }
> +  }
> +
> +  /**
> +   * Called when a rule from the Rule view was released from being selected for an editor.

Same rewording for above.

::: devtools/client/inspector/fonts/fonts.js:145
(Diff revision 3)
>      gDevTools.off("theme-switched", this.onThemeChanged);
>  
>      this.document = null;
>      this.inspector = null;
>      this.pageStyle = null;
> +    this.selectedRule = null;

Add this.ruleView = null

::: devtools/client/inspector/fonts/reducers/font-editor.js:9
(Diff revision 3)
> +
> +"use strict";
> +
> +const {
> +  SHOW_EDITOR,
> +  HIDE_EDITOR,

Move this above.

::: devtools/client/inspector/fonts/reducers/font-editor.js:13
(Diff revision 3)
> +  SHOW_EDITOR,
> +  HIDE_EDITOR,
> +} = require("../actions/index");
> +
> +const INITIAL_STATE = {
> +  isVisible: false,

We also want to add comments about what each state property means.

Whether or not the font editor is visible.

::: devtools/client/inspector/fonts/reducers/font-editor.js:19
(Diff revision 3)
> +  selector: "",
> +};
> +
> +let reducers = {
> +
> +  [SHOW_EDITOR](state, { selector }) {

These 2 actions can really just be handled one.
See https://searchfox.org/mozilla-central/source/devtools/client/inspector/boxmodel/reducers/box-model.js#21 as an example of toggling a state.

[UPDATE_EDITOR_VISIBILITY](state, { selector, isVisible }) 

We should clear the selector anyways when we hide the editor to maintain the correct state.

::: devtools/client/inspector/fonts/reducers/font-editor.js:24
(Diff revision 3)
> +  [SHOW_EDITOR](state, { selector }) {
> +    return { ...state, isVisible: true, selector };
> +  },
> +
> +  [HIDE_EDITOR](state) {
> +    return { ...state, isVisible: false };

This should also clear the selector.

::: devtools/client/inspector/fonts/types.js:82
(Diff revision 3)
>  exports.fontOptions = {
>    // The current preview text
>    previewText: PropTypes.string,
>  };
>  
> +exports.fontEditorData = {

Would generally prefer to also have these alphabetically sorted as well, but I see I didn't do that for fontVariationAxis.

::: devtools/client/inspector/fonts/types.js:83
(Diff revision 3)
>    // The current preview text
>    previewText: PropTypes.string,
>  };
>  
> +exports.fontEditorData = {
> +  // Visibility toggle for the font editor

Reword this to be:

Whether or not the font editor is visiible

::: devtools/client/inspector/reducers.js:18
(Diff revision 3)
>  exports.events = require("devtools/client/inspector/events/reducers/events");
>  exports.extensionsSidebar = require("devtools/client/inspector/extensions/reducers/sidebar");
>  exports.flexbox = require("devtools/client/inspector/flexbox/reducers/flexbox");
>  exports.fontOptions = require("devtools/client/inspector/fonts/reducers/font-options");
>  exports.fontData = require("devtools/client/inspector/fonts/reducers/fonts");
> +exports.fontEditorData = require("devtools/client/inspector/fonts/reducers/font-editor");

I think I would prefer to drop the Data bit and rename it to fontEditor, but I will leave it up to you.

::: devtools/client/themes/fonts.css:13
(Diff revision 3)
>    flex-direction: column;
>    width: 100%;
>    height: 100%;
>  }
>  
> +#sidebar-panel-fonteditor {

I think we can remove this from landing since this was mainly for your testing.
Attachment #8958606 - Flags: review?(gl)
Comment on attachment 8958606 [details]
Bug 1443846 - Add skeleton for font editor panel.

https://reviewboard.mozilla.org/r/227516/#review235326

> This might be something we want to store in the redux store in the future.

The `rule` is a heavy object with a few circular references inside (rule.editor -> editor.rule). We can inspect when there's a use case for storing it.

> I think we can remove this from landing since this was mainly for your testing.

I dropped the yellow background, but I'll keep the selector because I plan to use it in upcoming patches.
Comment on attachment 8958606 [details]
Bug 1443846 - Add skeleton for font editor panel.

https://reviewboard.mozilla.org/r/227516/#review235624

The changes you made to rules.js should've went into the previous commit.

::: devtools/client/inspector/fonts/components/FontsApp.js:43
(Diff revision 4)
>        {
>          className: "theme-sidebar inspector-tabpanel",
>          id: "sidebar-panel-fontinspector"
>        },
> -      dom.div(
> -        {
> +      this.props.fontEditor.isVisible ?
> +        FontEditor(fontEditorProps)

We don't really need fontEditorProps and fontOverviewProps variable since they are only used once. 

It would be simpler to just do 
const { 
  fontEditor,
  ... 
} = this.props

FontEditor({
  fontEditor,
})
Attachment #8958606 - Flags: review?(gl) → review+
Comment on attachment 8958605 [details]
Bug 1443846 - Add swatch to mark rule as selected and toggle font inspector panel.

https://reviewboard.mozilla.org/r/227514/#review235628

I am gonna clear my review for this commit since I already saw you made the changes in the other commit, and we can just stick with pbro's r+
Attachment #8958605 - Flags: review?(gl)
Comment on attachment 8958606 [details]
Bug 1443846 - Add skeleton for font editor panel.

https://reviewboard.mozilla.org/r/227516/#review235624

I know. I tried to move them hack in history, but got in a situation where they were overwritten by the next commit. So I decided to fold them in the next commit.

> We don't really need fontEditorProps and fontOverviewProps variable since they are only used once. 
> 
> It would be simpler to just do 
> const { 
>   fontEditor,
>   ... 
> } = this.props
> 
> FontEditor({
>   fontEditor,
> })

That's neat!
Attachment #8958605 - Flags: review?(gl)
Pushed by gabriel.luong@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/342a85114d4c
Part 1: Add swatch to mark rule as selected and toggle font inspector panel. r=pbro
https://hg.mozilla.org/integration/mozilla-inbound/rev/6795fe2cb8d5
Part 2: Add skeleton for font editor panel. r=gl
https://hg.mozilla.org/integration/mozilla-inbound/rev/5299fa36891d
Part 3: Add SVG swatch icon for font editor toggle. r=pbro
https://hg.mozilla.org/mozilla-central/rev/342a85114d4c
https://hg.mozilla.org/mozilla-central/rev/6795fe2cb8d5
https://hg.mozilla.org/mozilla-central/rev/5299fa36891d
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox61: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 61
Blocks: 1449891
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: