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

Color names are no longer available to colorButton_foreStyle #3940

Closed
markegli opened this issue Mar 19, 2020 · 2 comments · Fixed by #3977
Closed

Color names are no longer available to colorButton_foreStyle #3940

markegli opened this issue Mar 19, 2020 · 2 comments · Fixed by #3977
Assignees
Labels
good first issue Relatively easy to fix. This is a perfect issue if you are willing to create a Pull Request. plugin:colorbutton The plugin which probably causes the issue. status:confirmed An issue confirmed by the development team. type:feature A feature request.
Milestone

Comments

@markegli
Copy link

Type of report

Regression blocking my migration to CKEditor 4.12 or newer.

Provide detailed reproduction steps (if any)

I am migrating some code from pre 4.12 that uses the color name in colorButton_foreStyle. Unfortunately color name is not available in foreStyle starting at 4.12 due to the fix to #1478. The fix for that bug makes sense, but if would be nice if setStyle provided the color name to foreStyle in another variable (say, colorName).

Existing code:

config.colorButton_colors = 'subtext/777, red-text/dd4b39';
config.colorButton_foreStyle = {
  element: 'span',
  attributes: {'class': '#(color)'},
  overrides: [{
    element: 'span',
    attributes: {
      'class': /.text/
    }
  }]
};

Suggested possible code after fix:

config.colorButton_colors = 'subtext/777, red-text/dd4b39';
config.colorButton_foreStyle = {
  element: 'span',
  attributes: {'class': '#(colorName)'},
  overrides: [{
    element: 'span',
    attributes: {
      'class': /.text/
    }
  }]
};

Expected result

Able to make use of the color name from colorButton_foreStyle.

<span class="subtext">styled text</span>

Actual result

Only the hex value is available.

<span class="#777">styled text</span>

Other details

  • CKEditor version: 4.12 and newer
@markegli markegli added the type:bug A bug. label Mar 19, 2020
@f1ames
Copy link
Contributor

f1ames commented Mar 19, 2020

Thanks for reporting and digging into this issue @markegli!

From our perspective it would be the best to restore the previous behavior (so staying with color) to make upgrading CKEditor 4 to newer versions as smooth as possible.

Reproduction demo - https://codepen.io/f1ames/pen/PoqaMYN.

@f1ames f1ames added plugin:colorbutton The plugin which probably causes the issue. regression This issue is a regression. status:confirmed An issue confirmed by the development team. good first issue Relatively easy to fix. This is a perfect issue if you are willing to create a Pull Request. workload:medium labels Mar 19, 2020
@hub33k hub33k self-assigned this Mar 31, 2020
@jacekbogdanski
Copy link
Member

jacekbogdanski commented Apr 9, 2020

Ok, so after digging into the issue, it seems like the change about passing (color) property as a value, not color name has been done on purpose and previously treated as a bug (#1478). It overall makes sense, because it's less likely that such property would be used to compose class names. You can read information about this change in config.colorbutton_colors docs:

Since 4.12.0: Defining colors with names works in a different way. Colors names can be defined by colorName/colorCode. The color name is only used in the tooltip. The output will now use the color code. For example, FontColor/FF9900 will be displayed as the color #FF9900 in the selector, and will be output as #FF9900.

Back to the main topic:

The fix for that bug makes sense, but if would be nice if setStyle provided the color name to foreStyle in another variable (say, colorName).

I think it's the most reasonable approach here, so instead of reverting fix, let's introduce additional style definition property:

color - we are keeping changes introduced in 4.12, so color is passed as a value in hex notation
colorName - we are passing color name if defined by colorButton_colors by a user, otherwise nothing

It also means that the issue should be treated as a feature, not regression.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Relatively easy to fix. This is a perfect issue if you are willing to create a Pull Request. plugin:colorbutton The plugin which probably causes the issue. status:confirmed An issue confirmed by the development team. type:feature A feature request.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants