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

Record selection changes in history (undo) #17824

Merged
merged 4 commits into from Nov 20, 2019
Merged

Conversation

@ellatrix
Copy link
Member

ellatrix commented Oct 7, 2019

Description

Same as #16428, but adjusted after #16932.

Stores selection in the undo reducer. The following gif shows the result after two times undo:

undo-selection

How has this been tested?

Screenshots

Types of changes

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.
  • I've included developer documentation if appropriate.
@ellatrix ellatrix mentioned this pull request Oct 7, 2019
5 of 5 tasks complete
@ellatrix ellatrix force-pushed the try/blocks-selection-history-2 branch from 36c3145 to 1057917 Oct 8, 2019
@ellatrix ellatrix marked this pull request as ready for review Oct 8, 2019
@ellatrix ellatrix added this to the Gutenberg 6.7 milestone Oct 8, 2019
@ellatrix ellatrix requested review from epiqueras and mcsf Oct 8, 2019
Copy link
Contributor

epiqueras left a comment

Great work! I just have one question, otherwise it looks perfect.

@@ -84,54 +141,121 @@ describe( 'undo', () => {
await pressKeyWithModifier( 'primary', 'z' ); // Undo 3rd paragraph text.

expect( await getEditedPostContent() ).toBe( thirdBlock );

This comment has been minimized.

Copy link
@epiqueras

epiqueras Oct 8, 2019

Contributor

We should add a check before the first undo here.

This comment has been minimized.

Copy link
@ellatrix

ellatrix Oct 14, 2019

Author Member

It should be enough to only test selection after undo right? After typing, we already know for sure where the selection is.

This comment has been minimized.

Copy link
@epiqueras

epiqueras Oct 14, 2019

Contributor

After typing, we already know for sure where the selection is.

It wouldn't hurt to verify that it synced correctly.

@ellatrix ellatrix force-pushed the try/blocks-selection-history-2 branch from ffebda8 to 63de0ad Oct 14, 2019
@ellatrix

This comment has been minimized.

Copy link
Member Author

ellatrix commented Oct 14, 2019

Unsure why one of the selection tests is failing. Need to have another look into it.

@ellatrix ellatrix modified the milestones: Gutenberg 6.7, Gutenberg 6.8 Oct 23, 2019
@ellatrix ellatrix added this to the Gutenberg 6.9 milestone Oct 28, 2019
@epiqueras epiqueras mentioned this pull request Nov 7, 2019
0 of 4 tasks complete
@aduth aduth modified the milestones: Gutenberg 6.9, Gutenberg 7.0 Nov 11, 2019
@ellatrix ellatrix mentioned this pull request Nov 15, 2019
6 of 6 tasks complete
@ellatrix ellatrix force-pushed the try/blocks-selection-history-2 branch from 63de0ad to 6e47e7a Nov 18, 2019
@ellatrix

This comment has been minimized.

Copy link
Member Author

ellatrix commented Nov 18, 2019

Rebased the PR and fixed the e2e test failure (caused by a miscalculation). @epiqueras would you be up for having another look?

@ellatrix ellatrix requested a review from epiqueras Nov 18, 2019
Copy link
Contributor

epiqueras left a comment

This is looking great.

Not a blocker, but I think we could do with some "multi-selection" test cases.

@ellatrix

This comment has been minimized.

Copy link
Member Author

ellatrix commented Nov 20, 2019

Multi selection + undo/redo tests don't seem trivial to add since it doesn't seem to be fully working. It resets to a collapsed range. I think I'd prefer to merge this as is now, and polish the behaviour with follow up PRs. The PR in its current form is already many times better than the current behaviour in master.

@epiqueras

This comment has been minimized.

Copy link
Contributor

epiqueras commented Nov 20, 2019

Sounds good.

@epiqueras epiqueras merged commit 6c4b5e5 into master Nov 20, 2019
2 checks passed
2 checks passed
pull-request-automation
Details
Travis CI - Pull Request Build Passed
Details
@epiqueras epiqueras deleted the try/blocks-selection-history-2 branch Nov 20, 2019
@ellatrix

This comment has been minimized.

Copy link
Member Author

ellatrix commented Nov 21, 2019

Thanks for the review @epiqueras!

@youknowriad

This comment has been minimized.

Copy link
Contributor

youknowriad commented Nov 25, 2019

Did we check what issues can be closed with this. I think we might have a few of these.

@ellatrix

This comment has been minimized.

Copy link
Member Author

ellatrix commented Nov 25, 2019

@youknowriad I only found #12327.

@youknowriad

This comment has been minimized.

Copy link
Contributor

youknowriad commented Nov 25, 2019

Interesting, I'm pretty sure I saw issues about the caret not restored to the right position on undo. We'll find them in triage sessions anyway.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.