Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Table properties UI] Implement a rich color picker for table (and table cell) properties #6106

Closed
oleq opened this issue Jan 22, 2020 · 5 comments · Fixed by ckeditor/ckeditor5-table#244
Assignees
Labels
domain:ui/ux This issue reports a problem related to UI or UX. package:table type:feature This issue reports a feature request (an idea for a new functionality or a missing option).

Comments

@oleq
Copy link
Member

oleq commented Jan 22, 2020

📝 Provide a description of the new feature

A follow-up of #6049.

ATM

image

Should be

image

with a color palette (see: font color) popping up when the color is clicked. The rich color input should become the ckeditor5-ui component.


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

@oleq oleq added type:feature This issue reports a feature request (an idea for a new functionality or a missing option). package:table domain:ui/ux This issue reports a problem related to UI or UX. labels Jan 22, 2020
@oleq oleq changed the title [Table UI] Implement a rich color picker for table (and table cell) properties [Table properties UI] Implement a rich color picker for table (and table cell) properties Jan 22, 2020
@oleq oleq added this to the iteration 29 milestone Jan 23, 2020
@oleq oleq assigned oleq and panr Feb 12, 2020
jodator added a commit to ckeditor/ckeditor5-ui that referenced this issue Feb 12, 2020
Internal: Moved `normalizeColorOptions()` and `getLocalizedColorOptions()` from `ckeditor5-font` (see ckeditor/ckeditor5/issues/6106).
jodator added a commit to ckeditor/ckeditor5-font that referenced this issue Feb 12, 2020
Other: Moved `normalizeColorOptions()` and `getLocalizedColorOptions()` to `ckeditor5-ui` (see ckeditor/ckeditor5/issues/6106).

MINOR BREAKING CHANGE: `normalizeColorOptions()` and `getLocalizedColorOptions()` are no longer available in this package. You can import them from `@ckeditor/ckeditor5-ui/src/colorgrid/utils` instead.
@jodator
Copy link
Contributor

jodator commented Feb 12, 2020

I wanted to move the talk about configuration here (might be an immediate follow-up). Quoting @oleq :

A couple of questions regarding the color palette and table forms configuration. They all overlap, so in fact it’s a one question with a couple of sub–questions.
Q: Should we organize table props configs in such way they are future proof in case we want to allow configuring border styles in the future?

config.table.tableCellProperties.border = { colors: [], styles: [] }

vs

config.table.tableCellProperties.borderColors = []
config.table.tableCellProperties.borderStyles = [] (we don’t have this yet)

Q: Separate configs for each form or shared configs? In other words: do you thing people want to configure separate palettes (also border styles in the future) for tables and table cells?

config.table.tableCellProperties. ...
config.table.tableProperties. ...

vs

config.table.tableProperties.border.colors (or borderColors, see previous Q)

Q: Should we have separate palettes for border colors and background colors?

config.table.tableCellProperties = { border: { colors: [], ... }, backgroundColors: [] }

vs.

config.table.tableCellProperties = { colors: [], border: { ... } }

1. Namespaces

I think that we probably could match configuration option with the model names (command name):

config.table.tableProperties = {
    backgroundColors: [],
    borderColors: []
}

2. Fallbacks (default options).

Also, for me, it would be nice to provide some sane fallbacks so if one would like to override default configuration it would require to change only one value - assuming setting new palette for tables.

It might be nice to make tableCellProperties dependant on tableProperties values. This should obviously work only when both features are included. If only TableCellProperties is included in build it should set own (the same) defaults).

So taking the above example this setting would set also table.tableCellProperties.backgroundColor and table.tableCellProperties.borderColors.

Another thing that came to my mind is that maybe we could ease developers and provide another fallback so it would require only one value to change. Maybe backgroundColor config should propagate to borderColor? But OTOH this feels a bit wrong in a semantic way.

This fallback behavior is not so hard to do using variables passed to the config, but anyway some enhancement.

@Reinmar
Copy link
Member

Reinmar commented Feb 12, 2020

Also, for me, it would be nice to provide some sane fallbacks so if one would like to override default configuration it would require to change only one value - assuming setting new palette for tables.

I don't think this is necessary. If you want to change all options, you do that:

const colors = [ ... ];

Editor.create( el, {
     tableProperties: {
         backgroundColors: colors,
         borderColors: colors
     },
     tableCellProperties: {
         backgroundColors: colors,
         borderColors: colors
      }
} )

If anything, we could later introduce something like config.colorPicker.colors to configure the color picker across all features. But I don't think that any sort of mechanism is needed now. As long as you can achieve what you want to achieve, a bit more code is not a problem.

@panr
Copy link
Contributor

panr commented Feb 12, 2020

If anything, we could later introduce something like config.colorPicker.colors to configure the color picker across all features.

Like it 👍

@oleq
Copy link
Member Author

oleq commented Feb 12, 2020

I decided to have tableCellProperties.border.colors and tableCellProperties.backgroundColors in case we want to configure border styles in the future. There's nothing else that can be done with backgrounds, though. Do you think it's overcomplicated?

@Reinmar
Copy link
Member

Reinmar commented Feb 12, 2020

Do you think it's overcomplicated?

A bit, IMO.

This will work fine as well:

tableCellProperties.borderColors
tableCellProperties.borderStyles
tableCellProperties.backgroundColors

Less nesting, lest objects (interfaces) to describe. We won't have 100 options in tableCellProperties anyway, so grouping seems to be overkill to me.

This was referenced Apr 6, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
domain:ui/ux This issue reports a problem related to UI or UX. package:table type:feature This issue reports a feature request (an idea for a new functionality or a missing option).
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants