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

Accessibility Helper dialog supports multiple command keystrokes #2520

Merged
merged 5 commits into from
Oct 26, 2018
Merged

Conversation

jacekbogdanski
Copy link
Member

What is the purpose of this pull request?

Bug fix

Does your PR contain necessary tests?

All patches which 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

What changes did you make?

Included all command keystrokes if available.

I wonder if we need a unit test for proposed change. It would probably require searching for correct text by regex inside dialog HTML. WDYT?

Closes #2519

@mlewand mlewand self-requested a review October 26, 2018 15:15
@mlewand mlewand self-assigned this Oct 26, 2018
@mlewand mlewand self-assigned this Oct 26, 2018
@mlewand
Copy link
Contributor

mlewand commented Oct 26, 2018

I wonder if we need a unit test for proposed change. It would probably require searching for correct text by regex inside dialog HTML. WDYT?

I think this is a no-brainer so we can skip this one. But yea, basically your approach would be a nice way to test it.

Copy link
Contributor

@mlewand mlewand left a comment

Choose a reason for hiding this comment

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

Just a minor things that I'll change during the review.

@bender-ui: collapsed
@bender-ckeditor-plugins: wysiwygarea, a11yhelp, link

1. Focus the edtior.
Copy link
Contributor

Choose a reason for hiding this comment

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

typo editor


## Expected:

### Windows/Linux
Copy link
Contributor

Choose a reason for hiding this comment

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

Actually this could be simplified, you could type it just as a list (i mean expected/unexpected section), so it's more concise.


### Windows/Linux

Link command is described by two keystrokes: `CTRL+L / CTRL+K`.
Copy link
Contributor

Choose a reason for hiding this comment

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

Just a detail, the button order is reversed. Not a big deal but someone might get a ❤️ attack during the testing phase. 😉


// Return the keystroke representation or leave match untouched
// if there's no keystroke for such command.
Copy link
Contributor

Choose a reason for hiding this comment

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

The comment about intentionally returing unmatched string was somewhat helpful, coz it might look like a bug - while the intention is to leave placeholder not replaced.

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.

Multiple keyboard shortcuts (for a single command) should be displayed in a11yhelp
2 participants