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

Incorrect rendering of the color button on first opening #4687

Merged
merged 10 commits into from
May 19, 2021
Merged

Conversation

KarolDawidziuk
Copy link
Contributor

What is the purpose of this pull request?

Bug fix

Does your PR contain necessary tests?

All patches that change the editor code must include tests. You can always read more
on PR testing,
how to set the testing environment and
how to create tests
in the official CKEditor documentation.

This PR contains

  • Unit tests
  • Manual tests

Did you follow the CKEditor 4 code style guide?

Your code should follow the guidelines from the CKEditor 4 code style guide which helps keep the entire codebase consistent.

  • PR is consistent with the code style guide

What is the proposed changelog entry for this pull request?

* [#<4247>](https://github.com/ckeditor/ckeditor4/issues/4247): Fixed: [Color Button](https://ckeditor.com/cke4/addon/colorbutton) Incorrect rendering of the color button on first opening.

What changes did you make?

Removing scroll bar, that was cousing width calculation problem in the first render.

Which issues does your PR resolve?

Closes #4247.

@KarolDawidziuk KarolDawidziuk changed the title T/4247 Incorrect rendering of the color button on first opening May 10, 2021
@Comandeer Comandeer self-requested a review May 10, 2021 15:24
@Comandeer Comandeer self-assigned this May 10, 2021
Copy link
Member

@Comandeer Comandeer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The fix itself looks good 👍🏻 However there are some issues with tests (see inline comments).

plugins/colorbutton/plugin.js Show resolved Hide resolved
@@ -192,6 +192,9 @@
// The block should not have scrollbars (https://dev.ckeditor.com/ticket/5933, https://dev.ckeditor.com/ticket/6056)
block.element.getDocument().getBody().setStyle( 'overflow', 'hidden' );

// First render of panel have a scrollbar, but it shouldn't (https://github.com/ckeditor/ckeditor4/issues/4247)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In case of references to GH issues, we use just an issue number preceded by hash:

Suggested change
// First render of panel have a scrollbar, but it shouldn't (https://github.com/ckeditor/ckeditor4/issues/4247)
// First render of panel have a scrollbar, but it shouldn't (#4247).

'test background color items not draggable': testElementsNotDraggable( 'BGColor' ),

// (#4247)
'test panel should not have a scrollbar': function() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In fact you check here if the overflow: hidden is applied correctly, not the scrollbar itself, so maybe:

Suggested change
'test panel should not have a scrollbar': function() {
'test panel should have styles that prevent scrollbar appearance': function() {

removeButtons: 'BGColor'
removeButtons: 'BGColor',
// Config due to #4247
language: 'pl'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of modifying the existing manual test, please create a new one. The main benefit of it will be the fact that there will be the clear procedure how to reproduce the issue and what to check if it still exists.

Copy link
Member

@Comandeer Comandeer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! I've got only one small remark about the description of manual test.

@bender-ui: collapsed
@bender-ckeditor-plugins: wysiwygarea, toolbar, colorbutton, colordialog, sourcearea

1. Click **Text Color** or **Background Color** button.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Current procedure suggests that the test can be done only for the one button. But it's better to test both of the buttons. So I propose to make the first step (with expected and unexpected sections) for Text Color button and then the second step like:

2. Repeat the procedure for the **Background Color** button.

Copy link
Member

@Comandeer Comandeer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. I've just fixed some inconsistent indentation in manual test.

I'll wait with merging until master branch is unfrozen.

@Comandeer Comandeer changed the base branch from master to next May 19, 2021 08:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants