1518850 - Add the computed property list and shorthand overridden list

Status: RESOLVED FIXED

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

Comment 1

[Gabriel Luong [:gl] (ΦωΦ)]

Attachment: 1518850.patch [1.0]

Comment 2

[Gabriel Luong [:gl] (ΦωΦ)]

For reference, this patch implements the following https://searchfox.org/mozilla-central/source/devtools/client/inspector/rules/views/text-property-editor.js#623-741

Comment 3

[Razvan Caliman [:rcaliman]]

Comment on attachment 9035364 [details] [diff] [review]
1518850.patch [1.0]

Review of attachment 9035364 [details] [diff] [review]:
-----------------------------------------------------------------

There is potential to refactor the Declarations and Declaration components a bit so the rendering of CSS declarations can be reused for the regular, computed and overridden property lists. I tried to do this in a separate patch but got stuck. I leave it to you if you want to address this now or later.

The `.ruleview-propertylist` class can be used on all lists. The nesting of `.ruleview-propertylist .ruleview-propertylist` could be leveraged for indentation instead of relying separate class names. 

For the most part, `.ruleview-property` can be used on all declarations, but the overridden list items have additional styling which would still require `.ruleview-overridden-item` as a modifier.

::: devtools/client/inspector/rules/components/Declaration.js
@@ +45,5 @@
> +
> +    return (
> +      dom.ul(
> +        {
> +          className: "ruleview-computedlist",

`.ruleview-computedlist` can be replaced with `.ruleview-propertylist` with no change in styling.

@@ +55,5 @@
> +          return (
> +            dom.li(
> +              {
> +                key: `${name}${value}`,
> +                className: "ruleview-computed" +

`.ruleview-computed` can be replaced with `.ruleview-property` with minimal visual change. The only place where it's used is to slightly offset the child `.ruleview-namecontainer`:

https://searchfox.org/mozilla-central/source/devtools/client/themes/rules.css#168-170

```
.ruleview-computed > .ruleview-namecontainer {
  margin: 0;
}
```

The same could be achieved by relying on the nesting like:

```
.ruleview-property .ruleview-property > .ruleview-namecontainer {
  margin: 0;
}
```

::: devtools/client/inspector/rules/models/text-property.js
@@ +53,5 @@
> +      .filter(computed => computed.name !== this.name)
> +      .map(computed => {
> +        return {
> +          name: computed.name,
> +          isOverridden: computed.overridden,

Please add isOverridden in types.js for the array of computed properties in the declaration shape definition.

Comment 4

[Gabriel Luong [:gl] (ΦωΦ)]

(In reply to Razvan Caliman [:rcaliman] from comment #3)

Comment on attachment 9035364 [details] [diff] [review]
1518850.patch [1.0]

Review of attachment 9035364 [details] [diff] [review]:

There is potential to refactor the Declarations and Declaration components a
bit so the rendering of CSS declarations can be reused for the regular,
computed and overridden property lists. I tried to do this in a separate
patch but got stuck. I leave it to you if you want to address this now or
later.

The .ruleview-propertylist class can be used on all lists. The nesting of
.ruleview-propertylist .ruleview-propertylist could be leveraged for
indentation instead of relying separate class names.

For the most part, .ruleview-property can be used on all declarations, but
the overridden list items have additional styling which would still require
.ruleview-overridden-item as a modifier.

::: devtools/client/inspector/rules/components/Declaration.js
@@ +45,5 @@

• return (

•  dom.ul(
  

•    {
  

•      className: "ruleview-computedlist",
  

.ruleview-computedlist can be replaced with .ruleview-propertylist with
no change in styling.

@@ +55,5 @@

•      return (
  

•        dom.li(
  

•          {
  

•            key: `${name}${value}`,
  

•            className: "ruleview-computed" +
  

.ruleview-computed can be replaced with .ruleview-property with minimal
visual change. The only place where it's used is to slightly offset the
child .ruleview-namecontainer:

https://searchfox.org/mozilla-central/source/devtools/client/themes/rules.
css#168-170

.ruleview-computed > .ruleview-namecontainer {
  margin: 0;
}

The same could be achieved by relying on the nesting like:

.ruleview-property .ruleview-property > .ruleview-namecontainer {
  margin: 0;
}

I didn't find any success with this. I ran into padding-left issues that caused a visual regression as a result of https://searchfox.org/mozilla-central/source/devtools/client/themes/rules.css#533.

I kept it as-is for now. We will probably want to do a total overhaul of the CSS in the future.

::: devtools/client/inspector/rules/models/text-property.js
@@ +53,5 @@

•  .filter(computed => computed.name !== this.name)
  

•  .map(computed => {
  

•    return {
  

•      name: computed.name,
  

•      isOverridden: computed.overridden,
  

Please add isOverridden in types.js for the array of computed properties in
the declaration shape definition.

Fixed.

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b7e4f51b79a9
Add the computed property list and shorthand overridden list. r=rcaliman

Comment 6

[Narcis Beleuzu [:NarcisB]]

https://hg.mozilla.org/mozilla-central/rev/b7e4f51b79a9