[patch][cla] ColorPicker fixes and features

[mjekovec]

• set initial color and ability to set it later by calling a method
  
• color codes can now be entered through inputs (rgb, hsv, hex) and picker updates accordingly
  
• removed references to deprecated objects and methods
  
• rewritten it to use dojox.color instead of default _hsv2rgb method that was copy/pasted
  
• changed some method names to be more mnemomic
  
• commented the code
  
• most of it now conforms to JS Style Guide
  

[mjekovec]

Patch file for color picker

[dante]

see patch on #6627

[dante]

mjekovec - were you going to update this patch to exclude the post-build code (hasResource, copyright notice, and templateString) as we discussed in irc? Otherwise, minus a couple small style issues the patch seems fine.

[dante]

I'm confused about much in this patch, and going to close this as wontfix without response in 4 weeks. The style of the patch makes it difficult to see, but it actually breaks style guidelines (tabs to spaces, patched against a built/interned-string module). There are also a number of questionable changes to the widget size calculating that need to be reviewed, but unfortunately cannot effectively because of the "messyness" of the diff, which is more unfortunate because the patch seems to work fine, and addresses some great issues. please reopen if you are interested in resubmitting the patch, it is simply too "messy" to work with as it stands.

[Tom Elliott]

Hi Dante.

If you don't have the time to look at this I could try and re-patch the patch to conform to the coding standards to make your job easier. The fixes this patch puts in place are really useful for me! If you want me to have a look just drop me an email.

Also, I've found an error in the js file which prevents it working correctly in IE, there's an extra comma at line 283, after the "left: 0".

Tom

[dante]

Thanks bill for pinging me on this again, trac didn't.

@Tom - I'd love for you to adjust the patch. I agree it is a great feature set, and seemed to work (hadn't tested ie yet). If you could re-apply the changes to a trunk svn checkout maintaining tab spacings and templatePath etc, it will make the patch much easier to test and review.

also, obligatory "do you have a CLA on file?" ...

Awesome. Re-opening marked future. Will delegate to version when patch is ready. Thanks for stepping up.

[Tom Elliott]

Hi Dante. Nope, I don't have a CLA, what do I need to do?

Also, could you point me in the general direction of your coding standards. I'm new to Dojo so don't know quite what you need.

Thanks,

Tom

[Tom Elliott]

Also, is there a way of getting Trac to ping me when this ticket changes? I've had a look around and can't see anything obvious... :(

[dante]

@mrtom - for brevity because in trac:

• style guidelines: http://dojotoolkit.org/developer/StyleGuide - every tiny nuance about every little formatting quirk imaginable. the summary, and easy to remember parts: 1 tab indent, 4 spaces per tab, no whitespace between if/while/for type calls, whitespace between all vars and operations, functions are verbs (setSomething()).

• CLA info: ​http://dojofoundation.org/about/cla - required for all shipped and hosted contributions.

• getting notifications - login here, "preferences" (top right nav), enter username and supply email (kept hidden). any ticket you open, own, or are cc'd on with that username will be notified via email to that address.

• general community involvement overview: http://dojotoolkit.org/community

Feel free to jump in #dojo (irc.freenode.net, i'm phiggins) or ping me off-list if you have any questions or concerns. dante [at] dojotoolkit.org

[Tom Elliott]

Cleaned up version of previous patch

[Tom Elliott]

Hopefully the attached is a cleaner version of the changes which (mostly) conforms to the coding standards.

Where possible the changes in the file are *not* cosmetic, i.e. the patch attempts to show the code changes rather than coding style changes to ease code review. Once/If? the patch is accepted someone might want to go over it again and modify purely cosmetic changes.

Tom

[dante]

(In [16337]) fixes #8169 - awesome patch! (other than it coming in .rtf ;) ) Thanks mrtom! (cla) -- initial patch from mjekovec was foundation, and seemed to work, but needed cleanup seriously. ColorPicker? now works with .attr("#ededed"), sets it state to a valueon startup, monitor's onChange of each of the inputs and updates the hex value based on those changes, etc. Turning into a great widget. Tested IE6, but the height of the picker is totally wrong with no clear explanation. Still experimental. !strict

[abmalhot]

why does _setValueAttr() checks if the widget has started:

if(!this._started){ return; }

I believe this piece of code is unnecessary because invoking setColor explicitly works well even before widget startup so why are we preventing setColor to be invoked from within _setValueAttr

I ran into this issue while trying to set color to ColorPicker? in postCreate of my templated widget which uses ColorPicker?. and I noticed set('value', <color>) didnt work while setColor did.

[bill]

Since this ticket was closed three years ago you would be better off filing a new ticket, with a test case.

I think though that if() statement is there because _setValueAttr() will be called automatically during widget construction, assuming that a value was specified.