733747 - Highlight changed items in the rule view

Status: RESOLVED FIXED

(DevTools :: Inspector, defect, P2)

Description

[Kevin Dangoor]

To make it easy for a user to see what they've changed, we should visually distinguish the items in the rules view (and possibly the computed view as well)

Comment 1

[Paul Rouget [:paul]]

I understand for the computed view, but for the rule view, how is it useful? What kind of changes are you talking about?

Comment 2

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

The only place that you can make changes to the css are the rule view and style editors so it doesn't really make sense to have this for the computed view.

We think that highlighting changes using a green line down the gutter makes the most sense as this is how most code editors do it.

As far as implementation ... we currently save user entered changes in a store so highlighting the rows would be very simple as we are already keeping track of changed property values.

It is literally an hours work.

Comment 3

[Paul Rouget [:paul]]

(In reply to Michael Ratcliffe from comment #2)
> The only place that you can make changes to the css are the rule view and
> style editors so it doesn't really make sense to have this for the computed
> view.

I am not sure to understand. We are talking about a value being updated, right?

In the Rule View and the Computed Style, when does a value change?
When you edit the style in the Style Editor or in the Rule View, right?

Why not updating the Computed View while using the Style Editor?

Comment 4

[Kevin Dangoor]

(In reply to Paul Rouget [:paul] from comment #1)
> I understand for the computed view, but for the rule view, how is it useful?
> What kind of changes are you talking about?

The specific use case here is that the web developer is working on their design and they make a bunch of changes. Then, they want to get those changes into their original source files.

If we make the items they've changed visible, that will make it easier for them to make the changes in their original source.

In the Style Editor, they can save pretty easily (unless they're not editing the original source... eg they're working from minified files or originally had LESS/SASS). In the rule view, it's a bit harder.

(A global "diff" view that enumerates the changes they've made may be even more helpful. This is just one option.)

Comment 5

[Paul Rouget [:paul]]

hoooo, I see. I was totally off track :) Forget about my comments.

Comment 6

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Attachment: Patch

Very simple patch

Comment 7

[Mihai Sucan [:msucan]]

Comment on attachment 615636 [details] [diff] [review]
Patch

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

Looks good to me! Thanks for the patch!

Just a couple of comments below (addressing these is not mandatory for the review).

::: browser/devtools/styleinspector/CssRuleView.jsm
@@ +440,5 @@
>     * style's store.  Will re-mark overridden properties.
> +   *
> +   * @param {string} [aName]
> +   *        A text property name (such as "background" or "border-top") used
> +   *        when calling from setPropertyValue & setPropertyName to signify that

s/signify/flag/

(not sure, but doesn't "flag" fit better in this case?)

@@ +460,5 @@
>        }
>  
> +      if (aName && prop.name == aName) {
> +        store.userProperties.setProperty(this.style, prop.name, prop.value);
> +      }

This looks fine to me, but I wonder if this stuff would fit better in the TextProperty.setName/setValue() methods? What do you think?

I mean, why should we keep adding stuff to applyProperties()? Maybe we can keep it smaller.

Just a thought...

Comment 8

[Paul Rouget [:paul]]

https://hg.mozilla.org/integration/fx-team/rev/316c8e95720f

Comment 9

[Rob Campbell [:rc] (:robcee)]

https://hg.mozilla.org/mozilla-central/rev/316c8e95720f