#8169 closed enhancement (fixed)
[patch][cla] ColorPicker fixes and features
Reported by: | mjekovec | Owned by: | dante |
---|---|---|---|
Priority: | high | Milestone: | 1.3 |
Component: | Dojox | Version: | 1.2.2 |
Keywords: | ColorPicker, dojox | Cc: | |
Blocked By: | Blocking: |
Description
- 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
Attachments (2)
Change History (16)
Changed 12 years ago by
Attachment: | ColorPicker.patch added |
---|
comment:1 Changed 12 years ago by
Owner: | changed from Adam Peller to dante |
---|---|
Status: | new → assigned |
comment:3 Changed 12 years ago by
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.
comment:4 Changed 12 years ago by
Milestone: | 1.4 → future |
---|---|
Resolution: | → invalid |
Status: | assigned → closed |
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.
comment:5 Changed 12 years ago by
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
comment:6 Changed 12 years ago by
Resolution: | invalid |
---|---|
Status: | closed → reopened |
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.
comment:7 Changed 12 years ago by
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
comment:8 Changed 12 years ago by
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... :(
comment:9 Changed 12 years ago by
@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
Changed 12 years ago by
Attachment: | ColorPickerPatch.patch.rtf added |
---|
Cleaned up version of previous patch
comment:10 Changed 12 years ago by
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
comment:11 Changed 12 years ago by
Milestone: | future → 1.3 |
---|
comment:12 Changed 12 years ago by
Resolution: | → fixed |
---|---|
Status: | reopened → closed |
(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
comment:13 Changed 9 years ago by
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.
comment:14 Changed 9 years ago by
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.
Patch file for color picker