Opened 11 years ago

Closed 11 years ago

Last modified 7 years ago

#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)

ColorPicker.patch (31.0 KB) - added by mjekovec 11 years ago.
Patch file for color picker
ColorPickerPatch.patch.rtf (15.6 KB) - added by Tom Elliott 11 years ago.
Cleaned up version of previous patch

Download all attachments as: .zip

Change History (16)

Changed 11 years ago by mjekovec

Attachment: ColorPicker.patch added

Patch file for color picker

comment:1 Changed 11 years ago by dante

Owner: changed from Adam Peller to dante
Status: newassigned

comment:2 Changed 11 years ago by dante

Milestone: tbd1.4

see patch on #6627

comment:3 Changed 11 years ago by 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.

comment:4 Changed 11 years ago by dante

Milestone: 1.4future
Resolution: invalid
Status: assignedclosed

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 11 years ago by 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

comment:6 Changed 11 years ago by dante

Resolution: invalid
Status: closedreopened

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 11 years ago by 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

comment:8 Changed 11 years ago by 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... :(

comment:9 Changed 11 years ago by 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()).
  • 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.

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 11 years ago by Tom Elliott

Attachment: ColorPickerPatch.patch.rtf added

Cleaned up version of previous patch

comment:10 Changed 11 years ago by 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

comment:11 Changed 11 years ago by dante

Milestone: future1.3

comment:12 Changed 11 years ago by dante

Resolution: fixed
Status: reopenedclosed

(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 7 years ago by 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.

Last edited 7 years ago by abmalhot (previous) (diff)

comment:14 Changed 7 years ago by 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.

Note: See TracTickets for help on using tickets.