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

fix/keyboard-shortcut-multi-block-unselect #3436

Merged
merged 3 commits into from Nov 13, 2017

Conversation

Projects
None yet
2 participants
@vladanost
Contributor

vladanost commented Nov 11, 2017

Description

This PR is referencing this issue #3430

Added Escape key to keyboard shortcuts so multi-block unselect works via keyboard as well.

How Has This Been Tested?

Current JavaScript unit tests and e2e test are passing.

Additional info

This is probably for separate issue but since it's related I put the info here.
I have created a Cypress test for multi-block selection (not included here) but I have a problem there.
I’m using Docker Cypress image so this can be related to my setup.

This is the problem:
There seems to be no way to add another test.
Here tests are ordered 001-hello-gutenberg.js, 002-adding-blocks.js. I added another 003-multi-block-selection.js and in this order it is failing. If I name it so it is run first or second, it passes and the one after it fails.
The same happens with 002-adding-blocks.js, if I put it first for example, 001-hello-gutenberg.js fails. This happens even with only 2 original tests reordered (probably that’s why they are numbered to begin with). This is the error message I am getting when they are reordered:

CypressError: Timed out after waiting '60000ms' for your remote page to load.

Your page did not fire its 'load' event within '60000ms'.

You can try increasing the 'pageLoadTimeout' value in 'cypress.json' to wait longer.

Browsers will not fire the 'load' event until all stylesheets and scripts are done downloading.

When this 'load' event occurs, Cypress will continue running commands.

Because this error occurred during a 'before all' hook we are skipping the remaining tests in the current suite: 'Hello Gutenberg'

Changing 'pageLoadTimeout' does not make any difference.

I have not included 003-multi-block-selection.js so original tests pass.
I’m not sure how to proceed.

@youknowriad

This comment has been minimized.

Show comment
Hide comment
@youknowriad

youknowriad Nov 13, 2017

Contributor

@vladanost I'd love if you can include the test you wrote, so I can try it locally and see why it fails.
The order should be irrelevant. They are numbered, just to run simpler tests first.

By the way there's a built-in environment and cypress setup you can use to avoid having to setup it yourself https://github.com/WordPress/gutenberg/blob/master/CONTRIBUTING.md#local-environment and https://github.com/WordPress/gutenberg/blob/master/CONTRIBUTING.md#end-to-end-testing-integration-tests

Contributor

youknowriad commented Nov 13, 2017

@vladanost I'd love if you can include the test you wrote, so I can try it locally and see why it fails.
The order should be irrelevant. They are numbered, just to run simpler tests first.

By the way there's a built-in environment and cypress setup you can use to avoid having to setup it yourself https://github.com/WordPress/gutenberg/blob/master/CONTRIBUTING.md#local-environment and https://github.com/WordPress/gutenberg/blob/master/CONTRIBUTING.md#end-to-end-testing-integration-tests

@vladanost

This comment has been minimized.

Show comment
Hide comment
@vladanost

vladanost Nov 13, 2017

Contributor

@youknowriad Thanks. I have pushed the test but I already see that CI failed.

Contributor

vladanost commented Nov 13, 2017

@youknowriad Thanks. I have pushed the test but I already see that CI failed.

@youknowriad

This comment has been minimized.

Show comment
Hide comment
@youknowriad

youknowriad Nov 13, 2017

Contributor

👍 Nice test.

The travis CI error is unrelated to the test, it's just an eslint error. I suggest you install an eslint plugin for you favorite IDE to surface these errors while coding.

You can run npm run lint locally to see these errors.

Contributor

youknowriad commented Nov 13, 2017

👍 Nice test.

The travis CI error is unrelated to the test, it's just an eslint error. I suggest you install an eslint plugin for you favorite IDE to surface these errors while coding.

You can run npm run lint locally to see these errors.

@codecov

This comment has been minimized.

Show comment
Hide comment
@codecov

codecov bot Nov 13, 2017

Codecov Report

Merging #3436 into master will increase coverage by 0.21%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3436      +/-   ##
==========================================
+ Coverage    33.8%   34.01%   +0.21%     
==========================================
  Files         249      252       +3     
  Lines        6713     6703      -10     
  Branches     1207     1217      +10     
==========================================
+ Hits         2269     2280      +11     
+ Misses       3754     3730      -24     
- Partials      690      693       +3
Impacted Files Coverage Δ
editor/modes/visual-editor/index.js 0% <ø> (ø) ⬆️
blocks/library/gallery/index.js 25.92% <0%> (-5.9%) ⬇️
components/autocomplete/index.js 73.26% <0%> (-4.21%) ⬇️
editor/effects.js 47.61% <0%> (-3.7%) ⬇️
editor/actions.js 43.39% <0%> (-0.84%) ⬇️
editor/reducer.js 90.27% <0%> (-0.5%) ⬇️
blocks/library/freeform/old-editor.js 1.36% <0%> (-0.15%) ⬇️
blocks/library/button/index.js 20% <0%> (ø) ⬆️
blocks/library/image/block.js 0% <0%> (ø) ⬆️
editor/store-defaults.js 100% <0%> (ø) ⬆️
... and 8 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 564c4ef...decbd08. Read the comment docs.

codecov bot commented Nov 13, 2017

Codecov Report

Merging #3436 into master will increase coverage by 0.21%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3436      +/-   ##
==========================================
+ Coverage    33.8%   34.01%   +0.21%     
==========================================
  Files         249      252       +3     
  Lines        6713     6703      -10     
  Branches     1207     1217      +10     
==========================================
+ Hits         2269     2280      +11     
+ Misses       3754     3730      -24     
- Partials      690      693       +3
Impacted Files Coverage Δ
editor/modes/visual-editor/index.js 0% <ø> (ø) ⬆️
blocks/library/gallery/index.js 25.92% <0%> (-5.9%) ⬇️
components/autocomplete/index.js 73.26% <0%> (-4.21%) ⬇️
editor/effects.js 47.61% <0%> (-3.7%) ⬇️
editor/actions.js 43.39% <0%> (-0.84%) ⬇️
editor/reducer.js 90.27% <0%> (-0.5%) ⬇️
blocks/library/freeform/old-editor.js 1.36% <0%> (-0.15%) ⬇️
blocks/library/button/index.js 20% <0%> (ø) ⬆️
blocks/library/image/block.js 0% <0%> (ø) ⬆️
editor/store-defaults.js 100% <0%> (ø) ⬆️
... and 8 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 564c4ef...decbd08. Read the comment docs.

@vladanost

This comment has been minimized.

Show comment
Hide comment
@vladanost

vladanost Nov 13, 2017

Contributor

@youknowriad I added both modifiers. I see that Cypress will include OS utility methods cypress-io/cypress#824 Maybe that will be useful in some cases.

Contributor

vladanost commented Nov 13, 2017

@youknowriad I added both modifiers. I see that Cypress will include OS utility methods cypress-io/cypress#824 Maybe that will be useful in some cases.

@youknowriad

Nice fix and awesome job on the test. Thanks

const lastBlockSelector = '.editor-visual-editor__block-edit:last [contenteditable="true"]:first';
const firstBlockContainerSelector = '.editor-visual-editor__block:first';
const lastBlockContainerSelector = '.editor-visual-editor__block:last';
const multiSelectedCssClass = 'is-multi-selected';

This comment has been minimized.

@youknowriad

youknowriad Nov 13, 2017

Contributor

Raised before on other PRs. We should consolidate these selectors in shared variables or use data-test attributes

@youknowriad

youknowriad Nov 13, 2017

Contributor

Raised before on other PRs. We should consolidate these selectors in shared variables or use data-test attributes

@youknowriad youknowriad merged commit ff55187 into WordPress:master Nov 13, 2017

2 checks passed

codecov/project 34.01% (+0.21%) compared to 564c4ef
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@vladanost

This comment has been minimized.

Show comment
Hide comment
@vladanost

vladanost Nov 13, 2017

Contributor

@youknowriad Thanks.

Contributor

vladanost commented Nov 13, 2017

@youknowriad Thanks.

@vladanost vladanost deleted the vladanost:fix/keyboard-shortcut-multi-block-unselect branch Nov 16, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment