Closed Bug 1190047 Opened 4 years ago Closed 4 years ago

[Rule View] Swatch does not revert to the original value on escape

Categories

(DevTools :: Inspector, defect)

defect
Not set

Tracking

(firefox42 fixed)

RESOLVED FIXED
Firefox 42
Tracking Status
firefox42 --- fixed

People

(Reporter: gl, Assigned: gl)

References

Details

Attachments

(1 file, 3 obsolete files)

No description provided.
Assignee: nobody → gabriel.luong
Status: NEW → ASSIGNED
Attached patch 1190047.patch [1.0] (obsolete) — Splinter Review
Attachment #8642907 - Flags: review?(bgrinstead)
Blocks: 1188897
Attached patch 1190047.patch [2.0] (obsolete) — Splinter Review
Attachment #8642907 - Attachment is obsolete: true
Attachment #8642907 - Flags: review?(bgrinstead)
Attachment #8642915 - Flags: review?(bgrinstead)
Comment on attachment 8642915 [details] [diff] [review]
1190047.patch [2.0]

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

Looks good, tested locally and it works as expected.  Just one comment, see below

::: browser/devtools/styleinspector/rule-view.js
@@ +3133,5 @@
>          // knows about
>          this.ruleView.tooltips.colorPicker.addSwatch(span, {
> +          onShow: () => this._onStartEditing(),
> +          onPreview: () => this._onSwatchPreview(this.valueSpan.textContent),
> +          onCommit: () => {

We should be consistent with how we do all these callbacks.  Here's what I'd recommend:

    onShow: this._onStartEditing,
    onPreview: this._onSwatchPreview,
    onCommit: this._onSwatchCommit,
    onRevert: this._onSwatchRevert

This would require adding a new _onSwatchCommit function (that gets bound to `this` just like the other _onSwatch* functions), and removing the 'value' argument to them, instead just directly using `this.valueSpan.textContent` in those functions.

I think this will also make adding a new swatch type simpler
Attachment #8642915 - Flags: review?(bgrinstead)
Attached patch 1190047.patch [3.0] (obsolete) — Splinter Review
Attachment #8642915 - Attachment is obsolete: true
Attachment #8643288 - Flags: review?(bgrinstead)
Attachment #8643288 - Attachment is obsolete: true
Attachment #8643288 - Flags: review?(bgrinstead)
Attachment #8643300 - Flags: review?(bgrinstead)
Attachment #8643300 - Flags: review?(bgrinstead) → review+
https://hg.mozilla.org/mozilla-central/rev/78512934fdb8
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 42
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.