Closed Bug 1521243 Opened 2 years ago Closed 2 years ago

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

Categories

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

enhancement

Tracking

(firefox66 fixed)

RESOLVED FIXED
Firefox 66
Tracking Status
firefox66 --- fixed

People

(Reporter: gl, Assigned: gl)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

No description provided.
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+

(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!

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
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 66
You need to log in before you can comment on or make changes to this bug.