Add the computed property list and shorthand overridden list

RESOLVED FIXED in Firefox 66

Status

enhancement
P3
normal
RESOLVED FIXED
4 months ago
4 months ago

People

(Reporter: gl, Assigned: gl)

Tracking

(Blocks 1 bug)

unspecified
Firefox 66
Dependency tree / graph

Firefox Tracking Flags

(firefox66 fixed)

Details

Attachments

(1 attachment)

Assignee

Description

4 months ago
No description provided.
Assignee

Comment 1

4 months ago
Attachment #9035364 - Flags: review?(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.
Attachment #9035364 - Flags: review?(rcaliman) → review+
Assignee

Comment 4

4 months ago

(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

4 months ago
Pushed by gabriel.luong@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b7e4f51b79a9
Add the computed property list and shorthand overridden list. r=rcaliman

Comment 6

4 months ago
bugherder
Status: ASSIGNED → RESOLVED
Last Resolved: 4 months ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 66
You need to log in before you can comment on or make changes to this bug.