-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
Conversation
There was a problem hiding this 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?
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 |
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. |
There was a problem hiding this 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.
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.
This PR contains
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.
What is the proposed changelog entry for this pull request?
What changes did you make?
Changed
title = editor.lang.preview.preview,
totitle = editor.title,
Which issues does your PR resolve?
Closes #4026