Update Slider to reflect changes immediately

[pinarol]

Can we make Slider to reflect changes immediately without needing to wait for the hand gesture to end?

[pinarol]

hey @dratwas 👋 Can you start looking into this one?

[dratwas]

Hey @pinarol , sure :)

[dratwas]

I started to work on it and have one question here at this moment:

I use the spacer block as an example but it works the same for all blocks.

How should it work with the undo button? In the current implementation, the value is set to the attributes when we release the slider (onSlidingComplete) If i set the value in onValueChange then the value is set to the attributes many times and when i want to revert that change i need to click a lot of times on undo button.

I checked how it works on the web and to be honest, I'm not sure how it works, because sometimes it allows me to back to the previous value, and sometimes it just removes i.e spacer block.

I need to know the expected behavior :)
cc: @iamthomasbishop @pinarol

[pinarol]

Interestingly yes, it seems like web has different undo levels. It is observable on other blocks as well, like when I type in a paragraph block it undos the whole word on web. But, it removes letter by letter on mobile.

[pinarol]

I've asked this in the core-editor chat, linking the thread: https://wordpress.slack.com/archives/C02QB2JS7/p1589464864354700

[iamthomasbishop]

I need to know the expected behavior :)

@dratwas This is tricky. I think we should do 1 of the following 2 options the user opens the settings sheet to customize:

1. Only 1 undo step is applied when the user has dismissed the sheet. This means I could tap and drag multiple times and the only “step” that is saved is the value that is applied when I dismiss the sheet

2. An undo step is applied every time I drag & release. So if I tap and drag the handle 3 times, 3 separate undo steps are applied.

It’s also unclear to me what exactly the logic is for steps on web, because at first it seemed that it worked like my first bullet above, but then I tried again and it seemed to work like the second 🤷‍♂️. Can we first understand what their implemented logic is, and then decide whether it makes sense to diverge?

In my opinion, I think the second approach is more logical. I definitely wouldn’t apply a different undo step to every single point/increment on the slider, but instead rely on what the value is on dragEnd and create an undo step for that change.

[dratwas]

@iamthomasbishop

It’s also unclear to me what exactly the logic is for steps on web, because at first it seemed that it worked like my first bullet above, but then I tried again and it seemed to work like the second

Yeah, I observed the same and was a bit confused because of that.

I'm investigating how it works on the web and why it's different on the mobile.

In my opinion, I think the second approach is more logical. I definitely wouldn’t apply a different undo step to every single point/increment on the slider, but instead rely on what the value is on dragEnd and create an undo step for that change.

I totally agree, however, I'm not sure atm how it works exactly and if it is easy to implement in the current mechanism. I will come back with details about current behavior :)

[dratwas]

Hey, @iamthomasbishop, I spent some time checking how the undo works and why there is a difference between web and mobile. Seems like it works a bit differently than we thought. The undo level is created when we change something in a different block or different attribute in the same block. So in the spacer scenario, we can change the height of it and the undo level is not created since we change something else in the editor. The undo level is not created if we change only selection w/o changing any attribute. There is also one additional thing worth mentioning - I observed that sometimes I have a few undo levels on one block that changes only one attribute. It is possible in the scenario below:

• add a spacer (the undo level is created with default height)
• change its height
• add i.e paragraph (the undo level is created with current height)
• use the undo button (spacer is selected with a height that is set in the previous step)
• change height of the spacer
• add i.e paragraph (the next undo level is created with different height)

Now you have two undo levels that change the height of the spacer block one after the other.

cc @pinarol
I also found what is the difference in the implementation of web and mobile but I'm not sure why it is like that.

For the web, we fetch post type entities here https://github.com/WordPress/gutenberg/blob/master/packages/core-data/src/entities.js#L107
and we add the transient Edits to each entry:

transientEdits: {
	blocks: true,
	selectionStart: true,
	selectionEnd: true,
},

Then in the reducer that handles creating new undo levels, we check if we have edits that are not transient. https://github.com/WordPress/gutenberg/blob/master/packages/core-data/src/reducer.js#L418 and create a new undo level if yes. So if we edit the same attribute in the same block then in the action.edits we have only blocks, selectionStart and selectionEnd that means we will not create a new undo level because they are in transientEdits. If I change different attributes in the same block or some attribute in a different block then there is also content key in actions.edit and the undo level is created.

However, in the mobile, we always get error while fetching the postType so we return undefined here https://github.com/WordPress/gutenberg/blob/master/packages/core-data/src/entities.js#L109 and then we add transient edits in the provider - https://github.com/WordPress/gutenberg/blob/master/packages/editor/src/components/provider/index.native.js#L34 As you can see we do not add the same transientEdits as in the web case. There are missing selectionStart and selectionEnd keys which is the cause of creating the undo level on every change. When I added these keys then the undo mechanism seems to work the same as on the web but I'm not sure if I didn't break anything else.

Maybe someone has more context and could confirm that adding the selectionEnd and selectionStart to transientEdits on mobile doesn't break anything

[pinarol]

Not sure if I understood completely but are you saying we are going with Option 1 because there are some technical restrictions about fine-tuning undo levels on mobile? @dratwas

Option 1:

Only 1 undo step is applied when the user has dismissed the sheet. This means I could tap and drag multiple times and the only “step” that is saved is the value that is applied when I dismiss the sheet

[pinarol]

Maybe someone has more context and could confirm that adding the selectionEnd and selectionStart to transientEdits on mobile doesn't break anything

@SergioEstevao or @Tug may have more context. From what I have understand, it sounds reasonable to treat selectionEnd and selectionStart as transientEdits.

[dratwas]

Not sure if I understood completely but are you saying we are going with Option 1 because there are some technical restrictions about fine-tuning undo levels on mobile?

No, I meant that it is the current behavior on the web since @iamthomasbishop asked about it.

Can we first understand what their implemented logic is, and then decide whether it makes sense to diverge?

[pinarol]

I guess we know one thing for sure, we don't want to create 100 undo levels if the user changes changes the Slider 100 units. So it looks to me like we need to first experiment with undo levels to introduce transient edits. This would be better be done on a separate PR, and then we can base the Slider changes on that PR. How does it sound?

We have quite good amount of documented tests, I believe we can detect if we break something if we run them on the related PR.

https://github.com/wordpress-mobile/test-cases/blob/master/test-suites/gutenberg/writing-flow-sanity-check.md
https://github.com/wordpress-mobile/test-cases/blob/master/test-suites/gutenberg/sanity-tests.md

[dratwas]

This would be better be done on a separate PR, and then we can base the Slider changes on that PR. How does it sound?

Yeah, make sense. Let me prepare that :)