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

Use editor's title for new preview window #4915

Merged
merged 6 commits into from
Oct 4, 2021
Merged

Conversation

Elyasin
Copy link
Contributor

@Elyasin Elyasin commented Sep 28, 2021

What is the purpose of this pull request?

Other: It is a PR for the Issue 4026

Does your PR contain necessary tests?

No, it does not.

  1. I am not sure if this change requires a test.
  2. bender exits with a failure. I tried to follow the steps described in the corresponding documentation (Setting up tests, run tests, etc.). Too time consuming for this simple test.

This PR contains

  • None, as explained above

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.

  • [X ] PR is consistent with the code style guide

What is the proposed changelog entry for this pull request?

* [#4026](/~https://github.com/ckeditor/ckeditor4/issues/4026): Preview addon should use editor's "title" property for the title of the new window

What changes did you make?

Changed title = editor.lang.preview.preview, to title = editor.title,

Which issues does your PR resolve?

Closes #4026

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.

Hi, @Elyasin! Thanks for your contribution.

You're right, the change is a simple one, however, it still requires tests (ideally both automatic and manual ones). Do you want to devote some more time to create them or should I add them to this PR?

@Elyasin
Copy link
Contributor Author

Elyasin commented Sep 29, 2021

Hi @Comandeer - I tried to add a test. Unfortunately I could not get it running without errors. I followed the description about setting up testing and how to execute but bender errors out before even running the tests. I would need some help as I could not figure it out. I will try again and let you know soon.

@Elyasin
Copy link
Contributor Author

Elyasin commented Oct 1, 2021

I added an automatic test. I don't understand how manual tests word. Unfortunately it is not well explained. All I could understand is that they exist and there is a folder with two files for manual test in the preview test folder.

I decided to leave it as is for now. I'd need more support otherwise.

@Comandeer Comandeer self-assigned this Oct 4, 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.

@Elyasin, thanks for your contribution!

The unit test seems ok, I've just slightly adjusted it (I moved canceling event before resume to ensure it would be canceled even if the assertion failed and replace assert.isArray with assert.isMatching as it's an assertion dedicated to checking against regexes). I've also added a manual test.

As for manual tests, I'd like to think of them as tests intended for humans (where unit ones are for machines). The description of the testing procedure (e.g. what to click in the editor's toolbar and what to expect after the click) is put in the *.md file. The code that initializes the editor is in the *.html file. But you're right, it could be described more in our docs – I'll add an issue for it.

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.

Preview addon should use editor's "title" property for the title of the new window
2 participants