-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
Make it possible to undo prefix transforms #11497
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.
Seems like a good enhancement but I'd appreciate a second opinion as I'm not very familiar with this code.
Going to punt this for 4.4 as well. Tests are failing and this is just a way to divide work an ship. |
Yes, let’s merge the other undo PR first :)
…On Fri, 9 Nov 2018 at 13:06, Riad Benguella ***@***.***> wrote:
Going to punt this for 4.4 as well. Tests are failing and this is just a
way to divide work an ship.
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#11497 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AEfg60JtWO87z9-iQNHX7MJ0lwGUMRfFks5utWG-gaJpZM4YOqvO>
.
|
a848c93
to
4d7ae53
Compare
e831ce3
to
efb6a16
Compare
@@ -27,8 +27,7 @@ export const settings = { | |||
transforms: { | |||
from: [ | |||
{ | |||
type: 'pattern', |
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.
Should we schim the old API?
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.
I don't know. It was never official/documented.
This is still very hard to do as if you add a space it's still transformed, I wonder if the undo should reset to the value with a space in it. |
test/e2e/specs/blocks/list.test.js
Outdated
@@ -9,6 +9,7 @@ import { | |||
convertBlock, | |||
pressWithModifier, | |||
insertBlock, | |||
META_KEY, |
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.
It needs to be rebased with the master
branch.
With the latest refactoring, we are able to reuse logic from @wordpress/keycodes
package and express this using primary
modifier:
await pressWithModifier( 'primary', 'z' );
efb6a16
to
c81e843
Compare
I'm not certain I understand what's different between this PR and master in terms of behavior. |
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.
I noticed that this fixes some issues with focus jumps.
Haven't looked deeper, but probably some browsers, like Chrome, fail to visualise the trailing space. We might have to pad it with a non breaking space. |
Description
Fixes #11358.
This PR adjusts the prefix transforms so that they only execute when they match text right at the caret, so that they are undoable, and makes the check faster checking if the previous character is a space and not using any regular expressions otherwise on the whole text. Also adds e2e tests for all prefix and enter transforms, and adds some documentation.
How has this been tested?
Extra e2e tests have been added, but ensure e.g. list shortcuts like
1.
still work and it's possible to undo them and have it so that the prefix can be at the start without it being transformed.Screenshots
Types of changes
Checklist: