Last Comment Bug 733747 - Highlight changed items in the rule view
: Highlight changed items in the rule view
Status: RESOLVED FIXED
[ruleview][firefox14-wanted][fixed-in...
:
Product: Firefox
Classification: Client Software
Component: Developer Tools: Inspector (show other bugs)
: unspecified
: All All
: P2 normal (vote)
: Firefox 15
Assigned To: Michael Ratcliffe PTO -> Fri, 24 Jun 2016 [:miker] [:mratcliffe]
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-03-07 06:29 PST by Kevin Dangoor
Modified: 2012-05-11 11:31 PDT (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch (7.43 KB, patch)
2012-04-17 02:42 PDT, Michael Ratcliffe PTO -> Fri, 24 Jun 2016 [:miker] [:mratcliffe]
mihai.sucan: review+
Details | Diff | Review

Description Kevin Dangoor 2012-03-07 06:29:56 PST
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] 2012-03-07 06:33:42 PST
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 Michael Ratcliffe PTO -> Fri, 24 Jun 2016 [:miker] [:mratcliffe] 2012-03-07 08:53:25 PST
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] 2012-03-07 09:07:52 PST
(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 2012-03-12 10:13:58 PDT
(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] 2012-03-12 10:35:45 PDT
hoooo, I see. I was totally off track :) Forget about my comments.
Comment 6 Michael Ratcliffe PTO -> Fri, 24 Jun 2016 [:miker] [:mratcliffe] 2012-04-17 02:42:33 PDT
Created attachment 615636 [details] [diff] [review]
Patch

Very simple patch
Comment 7 Mihai Sucan [:msucan] 2012-04-17 10:17:36 PDT
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] 2012-05-11 02:09:49 PDT
https://hg.mozilla.org/integration/fx-team/rev/316c8e95720f
Comment 9 Rob Campbell [:rc] (:robcee) 2012-05-11 11:31:23 PDT
https://hg.mozilla.org/mozilla-central/rev/316c8e95720f

Note You need to log in before you can comment on or make changes to this bug.