[Webkit-unassigned] [Bug 71262] Web Inspector: Add colorpicker functionality to color swatches in Styles Sidebar

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=71262





--- Comment #40 from Brian Grinstead <briangrinstead at gmail.com>  2012-02-09 19:45:59 PST ---
(In reply to comment #39)
> (From update of attachment 125058 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=125058&action=review
> 
> Make sure all strings you pass into WebInspector.UIString(..) are present in Source/WebCore/English.lproj/localizedStrings.js (it is in UTF-16).

Done

> > Source/WebCore/ChangeLog:1
> > +2012-02-01  bgrins  <briangrinstead at gmail.com>
> 
> A real name will look nice here (http://trac.webkit.org/wiki/UsingGitWithWebKit#WebKitScriptsupportforGit has a good description of the git setup process for WebKit development)

Done

> > Source/WebCore/inspector/front-end/Spectrum.js:98
> > +WebInspector.Spectrum.hsvToRGBA = function(h, s, v, a) 
> 
> hsvaToRGBA?
> 
> > Source/WebCore/inspector/front-end/Spectrum.js:132
> > +WebInspector.Spectrum.rgbToHSVA = function(r, g, b, a) 
> 
> rgbaToHSVA?
>

Done

> > Source/WebCore/inspector/front-end/Spectrum.js:138
> > +    var max = Math.max(r, g, b), min = Math.min(r, g, b);
> 
> One variable per line
> 
> > Source/WebCore/inspector/front-end/Spectrum.js:254
> > +        if (rgb[3] === 1) {
> 

Done

> Please remove braces around single-line blocks
> 
> > Source/WebCore/inspector/front-end/Spectrum.js:257
> > +        else {
> 
> ditto
> 

Done

> > Source/WebCore/inspector/front-end/Spectrum.js:291
> > +        var round = [Math.round(rgb[0]), Math.round(rgb[1]), Math.round(rgb[2]), rgb[3]];
> 
> Rounding of the RGB values can be extracted into a separate method, since you do exactly the same in the "get color()" above.
> 

Moved to Spectrum.hsvaToRGBA

> > Source/WebCore/inspector/front-end/Spectrum.js:306
> > +    
> 
> a lonely extra blank line :)
> 

Ugh... finally fixed the setting on my text editor that was causing all of those problems with indenting new lines.  This lonely one was just from user error.

> > Source/WebCore/inspector/front-end/Spectrum.js:398
> > +        this.dispatchEventToListeners("hide", this.color);
> 
> Typically we define something along the lines of
> 
> WebInspector.Spectrum.Events = {
>     ColorChanged: "ColorChanged",
>     Hidden: "Hidden"
> }
> 
> (using the participle instead of infinitive) - see CSSStyleModel.js for an example.
> 
> Also, the Hidden event doesn't seem to need the color as the payload, since the updated color has been dispatched earlier in ColorChanged (and it's a good idea to dispatch the default initial color when the picker has just been opened.)
> 

Done

> > Source/WebCore/inspector/front-end/StylesSidebarPane.js:1644
> > +                    self.applyStyleText(nameElement.textContent + ": " + valueElement.textContent, true, true, false);
> 
> The user seems to be unable to cancel colorpicking (e.g. using Esc)?
> 

Should 'escape' close the picker in the current state or revert back to the original color from when it opened?

> > Source/WebCore/inspector/front-end/inspector.css:2690
> > +.spectrum-sat, .spectrum-val, .spectrum-top-inner { 
> 
> Instead of these separate classes, you can use the .fill class on respective elements, defined at the top of inspector.css. This will at least slightly help the size of our giant CSS.

Ah, good catch... fixed that too.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.