ColorInputView: displayed value should be converted back to a named color (if possible)

[Reinmar]

📝 Provide a description of the improvement

We could simply write "Green" here, cause that's the color that I picked.

---

If you'd like to see this improvement implemented, add a 👍 reaction to this post.

[tomalec]

Last week we discussed in P2P meeting with @Reinmar, that I will clean up the API of ColorGridView a little, by chaning https://github.com/ckeditor/ckeditor5/blob/master/packages/ckeditor5-ui/src/colorgrid/colorgridview.js#L109-L113

this.fire( 'execute', {
    value: item.color,
    hasBorder: item.options.hasBorder,
    label: item.label
 );

to just

this.fire( 'execute', item );

as this is precisely the one of given items that was selected.

Preferably as a separate micro PR, as it affects also some other components that delegate this event.

---

I also thought about a small refactoring. I'd like to change ColorDefinition's color property name to value. I see two reasons for such change:

1. It will simplify the usage, as currently, a few components repeatedly create new objects to translate color to value. What gives computation and cognitive overhead.
2. If "Color definition" defines a "color object" then the property that represents actual CSS value for it should be called: "color's color" or "color's value" - to me the latter sounds more natural.

@Reinmar WDYT?

---

Both changes would be breaking to the (partially undocumented) API.

[Reinmar]

I also thought about a small refactoring. I'd like to change ColorDefinition's color property name to value. I see two reasons for such change:

Doesn't that change the feature config format? That'd be a significant cost, so unless that's a blocker (I read it's not) I wouldn't do that.

[tomalec]

I think the major cost in confusing, inconsistent API here. We would end up with:

1. No change in existing stack, just passing color item as needed for this issue:
   • input in format for all bellow {color: 'rgb(255,0,0)', label: 'czerwony', options: {hasBorder: true}}
   • ColorGridView@execute: {value: 'rgb(255,0,0)', label: 'czerwony', hasBorder: true}, colorItem = {color: 'rgb(255,0,0)', label: 'czerwony', options: {hasBorder: true}} (confusing, redundant data emited)
   • ColorTableView@execute: delegated ColorGridView@execute, or {value: null}

or

2. Changing ColorGridView@execute to emit reference to the selected item, (as described in first part of https://github.com/ckeditor/ckeditor5/issues/6241#issuecomment-623920316)
   • input in format for all bellow {color: 'rgb(255,0,0)', label: 'czerwony', options: {hasBorder: true}}
   • ColorGridView@execute: reference to the item from the input. (⚠️ This is still a breaking change to undocumented API)
   • ColorTableView@execute: delegated {value: 'rgb(255,0,0)', label: 'czerwony', hasBorder: true} (This will require cloning an object just to rename the structure, just to preserver existing, still inconsistent API), or {value: null}

[tomalec]

There is also an option

3. Keep config format as is, but change more execute calls. Maybe as it is undocumented, it's not a cost yet.

   It means changing the data emitted by ColorGridView, ColorTableView ({value,...} to {color,...}), and make rewriting and inconsistency in ColorUI between ColorTableView and the dropdown.

[tomalec]

While waiting for your opinion, I continue solving OP with approach no. 1 - the ugliest in my opinion, but the easiest to merge.

[Reinmar]

I'm trying to understand one thing here. I understand that the API is messed up and that's confusing for developers. What I don't get is – will this change affect the config of end user features like font colors? Because that would be a significant breaking change for them and that's a big cost for the project.

[tomalec]

I'm trying to understand one thing here. I understand that the API is messed up and that's confusing for developers. What I don't get is – will this change affect the config of end user features like font colors?

It will. So, changing ColorDefinition structure is no longer a "small refactoring", I agree. I'll work today, on a solution that does not change ColorDefinition at all, and limits API changes to the minimum.

My only doubt now is how much should I bother to preserve an undocumented API - like execute event data (https://github.com/ckeditor/ckeditor5/issues/6743). But in the first step, I'd try to avoid changing this as well.

[tomalec]

Followups:

• [ ] https://github.com/ckeditor/ckeditor5-dev/issues/633
• [ ] https://github.com/ckeditor/ckeditor5/issues/6790
• [ ] https://github.com/ckeditor/ckeditor5/issues/6791