Opened 7 years ago

Closed 6 years ago

Last modified 6 years ago

#16908 closed feature (fixed)

Give TextColor plugin (foreColor and hiliteColor) support for custom color picker

Reported by: Nick Fenwick Owned by: bill
Priority: undecided Milestone: 1.10
Component: Editor Version: 1.9.0a2
Keywords: Cc:
Blocked By: #16930 Blocking:

Description

Same as #16881 but for dijit/_editor/plugins/TextColor.

Attachments (6)

16908.dojox.patch (523 bytes) - added by Nick Fenwick 7 years ago.
Patch on dojox just to fix dojox/widget/ColorPicker's habit of always firing onChange when value is set.
16908.dijit.patch (7.8 KB) - added by Nick Fenwick 7 years ago.
Patch on dijit to give _editor/plugins/TextColor.js custom colour picker ability.
test_TextColor.html (4.0 KB) - added by Nick Fenwick 7 years ago.
New tests file to test behaviour of font and hilite plugins in a variety of situations.
16908.dijit.patch2 (11.7 KB) - added by Nick Fenwick 7 years ago.
Replacement for earlier dijit patch and new files. Patches TextColor?.js, includes tests file, includes new file ConfirmTooltipDialog?.js.
16908.dijit.patch3 (12.0 KB) - added by Nick Fenwick 7 years ago.
Latest patch, supercedes patch2
TextColor_colorPickerProp.patch (2.4 KB) - added by bill 7 years ago.
adds colorPicker property to TextColor? plugin; it can point to a ColorPalette? or a ConfirmTooltipDialog? containing a dojox/widget/ColorPicker, where get("value") and set("value") delegate to that ColorPicker?

Download all attachments as: .zip

Change History (18)

comment:1 Changed 7 years ago by Nick Fenwick

I'm starting work on this now, no patch available yet.

It seems dojox/editor/plugins/TextColor is basically the same as dijit/_editor/plugins/TextColor but with a different dropdown. First glace indicates this work would deprecate the dojox plugin, as we would be able to use the core dijit one configured to use whatever colour picker / dropdown you want.

comment:2 Changed 7 years ago by bill

Makes sense, in that case dojox/editor/plugins/TextColor becomes a trivial (like 3 line) class.

Changed 7 years ago by Nick Fenwick

Attachment: 16908.dojox.patch added

Patch on dojox just to fix dojox/widget/ColorPicker's habit of always firing onChange when value is set.

Changed 7 years ago by Nick Fenwick

Attachment: 16908.dijit.patch added

Patch on dijit to give _editor/plugins/TextColor.js custom colour picker ability.

Changed 7 years ago by Nick Fenwick

Attachment: test_TextColor.html added

New tests file to test behaviour of font and hilite plugins in a variety of situations.

comment:3 Changed 7 years ago by Nick Fenwick

Just attached three files.

  1. simple patch on dojox. When using dojox/widget/ColorPicker as an immediate popup from the font color picker, it appears, has its value set with .set('value', whatever, false), and immediately fires onChange and the popup closes, because ColorPicker?'s _setValueAttr ignored the fireOnChange argument.
  2. and 3/ I'm being a bit of a cretin and can't see how to svn diff including my new tests file.

Because both a dropdown class and a picker class can be provided and the old TextColor? plugin supported async loading via require(), I have used dojo/promise/all to manage the two classes being loaded if necessary.

One problem stands out, if you drop down the plugins on the last example Editor, I have thrown in a very ugly example custom picker frame dijit, with TooltipDialog? .. its tooltipConnector is badly placed and I can't see why offhand. The overall popup seems to work fine.

Changed 7 years ago by Nick Fenwick

Attachment: 16908.dijit.patch2 added

Replacement for earlier dijit patch and new files. Patches TextColor?.js, includes tests file, includes new file ConfirmTooltipDialog?.js.

comment:4 Changed 7 years ago by Nick Fenwick

Just attached new patch. Please keep the original patch on dojox, that's still a good fix.

This patch includes work discussed with bill on IRC. Rather than port across the TablePlugins? internal dijit that allowed us to embed dojox/widget/ColorPicker in a TooltipDialog? with Set adn Cancel buttons, create a new dijit dijit/ConfirmTooltipDialog that can house any generic dijit that has a value.

Improvements over the old patch include:

  1. No longer need internal dijit to TextColor?.js
  2. From 1., no longer need to copy across nls/TextColor.js and all the associated translations, just to support the Set/Cancel? buttons on the dialog.
  3. No longer have concept of "default" dropDownClass. Just pass in "dijit/ConfirmTooltipDialog" or its constructor to use it.

We should look at updating TablePlugins? to get rid of CellColorDropDown? and use this dijit instead.

comment:5 Changed 7 years ago by bill

Owner: set to bill
Status: newassigned

Thanks for the patch. I've been working on this but not sure it will make it into 1.9.

About ConfirmTooltipDialog, it wasn't laying out correctly (with the connector) because the new widget didn't have an orient() method like TooltipDialog does. The larger issue was that ConfirmTooltipDialog was a new widget with an *embedded* TooltipDialog, rather than an extension of TooltipDialog. So I had to change that.

With the new ConfirmTooltipDialog, one can easily construct a TooltipDialog containing a dojox/widget/ColorPicker and OK/Cancel buttons that's a functional replacement for dijit/ColorPalette. I.E. you can easily make a widget that the TextColor plugin can use in place of dijit/ColorPalette. Therefore I don't currently see the need for changing the TextColor plugin beyond adding the ability to specify which color picker widget to use.

There are two differences between using a TooltipDialog vs. a ColorPalette, but TextBox can compensate for them:

  1. TooltipDialog has an onExecute() method instead of an onChange() method.
  1. TooltipDialog, like Dialog, extends _FormMixin, meaning that it expects to have children with name=... settings, and get("value") returns a structure like {field1: va1, field2: val2}. I don't see a reason to deviate in the case of ConfirmTooltipDialog

So, if the TextColor plugin sees the dropdown has an onExecute() method it will connect to that, and assume get("value") returns something like {color: "#ff0000"} rather than "#ff0000".

Changed 7 years ago by Nick Fenwick

Attachment: 16908.dijit.patch3 added

Latest patch, supercedes patch2

comment:6 Changed 7 years ago by Nick Fenwick

I asked on IRC but got no answer.. bill, if this entire patch cannot make it into 1.9 because of the new dijit/ConfirmTooltipDialog, but the dijit/_editor/plugins/TextColor work is basically good, can we please split the patch so we can commit TextColor? for 1.9, adn leave ConfirmTooltipDialog? as a separate goodie for later?

TextColor? has been modified to work with any popup, it doesn't rely on ConfirmTooltipDialog?, that's just an option one can use to house the picker.

We'd have to remove ConfirmTooltipDialog? from the tests file but TextColor? itself doesn't depend on it.

Also, I need the ability for TextColor?'s popup picker to be able to access the plugin's data. My patch3 includes that, passing params in. This is the same as was done on dojox/editor/plugins/TablePlugins. I'd be happy just passing a reference to "parent: this" or similar, but whatever, we really need that ability. Perhaps if you make your code available I can have a look and feed back.

comment:7 Changed 7 years ago by bill

Blocked By: 16930 added
Milestone: tbd2.0

No, sorry, it's passed the deadline for enhancements and also I don't want to add custom code in TextColor to create a TooltipDialog (i.e. the "dropDownClass" parameter) because that code won't be used by dijit at all.

I created #16930 to track the ConfirmTooltipDialog class. Note though that it works like TooltipDialog, i.e. like a dijit/form/Form. You can of course have just a single child widget, but I didn't build-in the code for get("value") to return this.getChildren()[0].get("value"), since that's specific to the Editor usage. Maybe consider adding it later.

Changed 7 years ago by bill

adds colorPicker property to TextColor? plugin; it can point to a ColorPalette? or a ConfirmTooltipDialog? containing a dojox/widget/ColorPicker, where get("value") and set("value") delegate to that ColorPicker?

comment:8 Changed 7 years ago by Nick Fenwick

OK thank you bill. I'm not clear on what's going to get into 1.9 now. We may have to copy the TextColor? plugin and not use the core dijit one in our project, as we cannot wait for this functionality. Still, this is valuable work in general. I had my main SSD drive fail two days ago which has buggered my ability to keep up with this work.

comment:9 Changed 7 years ago by bill

I'm sure you can just extend TextColor and override _initButton(), although making a copy is fine too. I'm not planning to put anything into 1.9 that isn't already there except for bug fixes.

comment:10 Changed 7 years ago by bill

Milestone: 2.01.10

Might as well check this into SVN and then merge to github, so it's available for the 1.x and 2.x streams.

comment:11 Changed 6 years ago by Bill Keese <bill@…>

Resolution: fixed
Status: assignedclosed

In 457e674b0757dda57db3c7aee27aa45f85dab719/dijit:

Error: Processor CommitTicketReference failed
Unsupported version control system "git": Can't find an appropriate component, maybe the corresponding plugin was not enabled? 

comment:12 Changed 6 years ago by Bill Keese <bill@…>

In 9f3e01c319cf418baaf709dab53b5f126097a05d/dojox:

Error: Processor CommitTicketReference failed
Unsupported version control system "git": Can't find an appropriate component, maybe the corresponding plugin was not enabled? 
Note: See TracTickets for help on using tickets.