Show a warning for invalid declarations and filter icon for overridden declarations in the new rules view

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 #9037727 - Flags: review?(mtigley)
Comment on attachment 9037727 [details] [diff] [review]
1521243.patch [1.0]

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

This looks good to me. One question though: could we add a test for this? Or will this be done in a separate bug?

::: devtools/client/inspector/rules/reducers/rules.js
@@ +33,5 @@
>      // An unique CSS declaration id.
>      id: declaration.id,
> +    // Whether or not the declaration is valid. (Does it make sense for this value
> +    // to be assigned to this property name?)
> +    isDeclarationValid: declaration.isValid(),

This makes sense to me.
Attachment #9037727 - Flags: review?(mtigley) → review+
Assignee

Comment 4

4 months ago

(In reply to Micah Tigley [:mtigley] from comment #3)

Comment on attachment 9037727 [details] [diff] [review]
1521243.patch [1.0]

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

This looks good to me. One question though: could we add a test for this? Or
will this be done in a separate bug?

::: devtools/client/inspector/rules/reducers/rules.js
@@ +33,5 @@

 // An unique CSS declaration id.
 id: declaration.id,
  • // Whether or not the declaration is valid. (Does it make sense for this value
  • // to be assigned to this property name?)
  • isDeclarationValid: declaration.isValid(),

This makes sense to me.

We would be ideally be reusing the existing tests in the rules view, which will covers this. The plan is to get the frontend working and adjust all the existing unit tests pass on the new frontend then flip the switch to the new frontend.

(In reply to Gabriel [:gl] (ΦωΦ) from comment #4)

(In reply to Micah Tigley [:mtigley] from comment #3)

Comment on attachment 9037727 [details] [diff] [review]
1521243.patch [1.0]

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

This looks good to me. One question though: could we add a test for this? Or
will this be done in a separate bug?

::: devtools/client/inspector/rules/reducers/rules.js
@@ +33,5 @@

 // An unique CSS declaration id.
 id: declaration.id,
  • // Whether or not the declaration is valid. (Does it make sense for this value
  • // to be assigned to this property name?)
  • isDeclarationValid: declaration.isValid(),

This makes sense to me.

We would be ideally be reusing the existing tests in the rules view, which will covers this. The plan is to get the frontend working and adjust all the existing unit tests pass on the new frontend then flip the switch to the new frontend.

Sounds good to me. Thanks for clarifying!

Comment 6

4 months ago
Pushed by gabriel.luong@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/65e0b8a04d1a
Show a warning for invalid declarations and filter icon for overridden declarations in the new rules view. r=mtigley

Comment 7

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.