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

[Tiny PR] Text patterns: fix undo after mouse move #18533

Merged
merged 1 commit into from
Nov 18, 2019
Merged

Conversation

ellatrix
Copy link
Member

@ellatrix ellatrix commented Nov 14, 2019

Description

Fixes #18492.

How has this been tested?

  1. Type 1. inside a paragraph. The block should convert to a list block.
  2. Move the mouse outside the block.
  3. Press Backspace. The block transform should be undone.

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.
  • I've updated all React Native files affected by any refactorings/renamings in this PR. .

@ellatrix ellatrix added [Type] Bug An existing feature does not function as intended [Package] Block editor /packages/block-editor labels Nov 14, 2019
@ellatrix ellatrix added this to the Gutenberg 7.0 milestone Nov 14, 2019
// Undoing an automatic change should still be possible after mouse
// move.
case 'STOP_TYPING':
return state;
}

// Reset the state by default (for any action not handled).
Copy link
Contributor

Choose a reason for hiding this comment

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

I think the root problem here is that this is dangerous in reducers in general. I'm pretty sure this error can happen again if we introduce new random actions. That said, now we have an e2e test to be aware of it at least.

Copy link
Member Author

Choose a reason for hiding this comment

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

Right, a newly introduced action might need to be added here, but it's more likely that it shouldn't be added here. If we do the opposite, it might be that we do the wrong action on Backspace, which I think is worse. I think it's better to whitelist the actions that should be ignored (which there are way less of).

Copy link
Contributor

@youknowriad youknowriad left a comment

Choose a reason for hiding this comment

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

While this fixes the issue, the fact that the caret is not in the paragraph after the undo is disturbing but I guess this is a different issue.

@ellatrix
Copy link
Member Author

@youknowriad Right, there a separate PR for that: #17824. Looks like one e2e test needs to be fixed and needs a rebase, but is otherwise looking good. I'll try to fix it sometime soon.

@ellatrix
Copy link
Member Author

A transformation e2e tests seem to be consistently failing... and I have no idea why. It seems to be around the full site editing blocks. @jorgefilipecosta or @epiqueras Do you have any idea?

FAIL packages/e2e-tests/specs/experimental/block-transforms.test.js (383.667s)
997  ● Block transforms › should contain the expected transforms
998
999    expect(received).toEqual(expected)
1000
1001    Difference:
1002
1003    - Expected
1004    + Received
1005
1006    @@ -374,10 +374,22 @@
1007            "Preformatted",
1008            "Verse",
1009          ],
1010          "originalBlock": "Paragraph",
1011        },
1012    +   "core__post-content": Object {
1013    +     "availableTransforms": Array [
1014    +       "Group",
1015    +     ],
1016    +     "originalBlock": "Post Content",
1017    +   },
1018    +   "core__post-title": Object {
1019    +     "availableTransforms": Array [
1020    +       "Group",
1021    +     ],
1022    +     "originalBlock": "Post Title",
1023    +   },
1024        "core__preformatted": Object {
1025          "availableTransforms": Array [
1026            "Group",
1027            "Paragraph",
1028          ],

@jorgefilipecosta
Copy link
Member

Hi @ellatrix, I'm submitting a fix.

@epiqueras
Copy link
Contributor

Thank you @jorgefilipecosta !

Sorry, I read the failures too quickly and thought they were the unrelated group transform failures PRs always get.

@ellatrix
Copy link
Member Author

Thanks for the review, @youknowriad!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Package] Block editor /packages/block-editor [Type] Bug An existing feature does not function as intended
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Typing 1, followed by a dot, then space forces a list
4 participants