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

Track block movement actions #4833

Merged
merged 1 commit into from
May 11, 2022
Merged

Track block movement actions #4833

merged 1 commit into from
May 11, 2022

Conversation

geriux
Copy link
Contributor

@geriux geriux commented May 5, 2022

Fixes #4831

Note: The bundle and Gutenberg reference changes are included to be able to test the drag & drop event, those will not be included once this is approved and ready to be merged.

This PR adds a new event editor_block_moved that is triggered when a block is moved by either:

  • Tapping on the up/down arrow button in the block toolbar.
  • Long-pressing on the up/down arrow button and moving a block to the top or bottom.
  • Drag & drop

The event has the following properties:

  • action_source which can be:
    • move_arrows_up when a block is moved using the up arrow button.
    • move_arrows_down when a block is moved using the down arrow button.
    • move_arrows_to_top when a block is moved to the top by long-pressing on the up arrow button.
    • move_arrows_to_bottom when a block is moved to the bottom by long-pressing on the down arrow button.
    • drag_and_drop when a block is moved using the drag & drop blocks feature.
  • block_name name of the block that is being moved.
  • inner_block if it has inner blocks or not.

Once this is merged, it will only track when a block is moved using the up/down arrow buttons, since the drag & drop blocks feature is not merged yet.

To test

Builds:

Move a block by tapping on the up/down arrow buttons

  • Open the app using the builds linked above.
  • Create a new post/page
  • Add some blocks
  • Select a block and tap the up/down arrow buttons within the block toolbar.
  • Using the live event page, expect the event wp(ios|android)_editor_block_moved to arrive successfully with the properties action_source containing: move_arrows_(up|down), inner_block and, block_name.

Move a block by long-pressing on the up/down arrow buttons

  • Open the app using the builds linked above.
  • Create a new post/page
  • Add some blocks
  • Select a block and long-press on the up/down arrow buttons within the block toolbar.
  • Select either Move to the top or Move to the bottom.
  • Using the live event page, expect the event wp(ios|android)_editor_block_moved to arrive successfully with the properties action_source containing: move_arrows_to_(top|bottom), inner_block and, block_name.

Move a block using the drag & drop blocks feature

  • Open the app using the builds linked above.
  • Create a new post/page
  • Add some blocks
  • Drag & drop a block
  • Using the live event page, expect the event wp(ios|android)_editor_block_moved to arrive successfully with the properties action_source containing: drag_and_drop, inner_blockand,block_name`.

PR submission checklist:

  • I have considered adding unit tests where possible.
  • I have considered if this change warrants user-facing release notes more info and have added them to RELEASE-NOTES.txt if necessary.

@peril-wordpress-mobile
Copy link

peril-wordpress-mobile bot commented May 6, 2022

Wanna run full suite of Android and iOS UI tests? Click here and 'Approve' CI job!

@geriux geriux changed the base branch from feature/drag-and-drop to trunk May 6, 2022 14:20
@geriux geriux requested a review from fluiddot May 9, 2022 12:27
@geriux geriux marked this pull request as ready for review May 9, 2022 12:27
@geriux geriux added this to the 1.76.0 milestone May 9, 2022
Copy link
Contributor

@fluiddot fluiddot left a comment

Choose a reason for hiding this comment

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

Amazing job @geriux 🏅 !

I confirm that the editor is sending editor_block_moved events for both actions of tapping on up/down arrows and drag & drop 🎊 .

However, I noticed that moving a block by long-pressing over the up/down arrows and tapping on "Move to Top" or "Move to bottom" is not generating an event. Not sure if these options are heavily used by users, but it might lead to receiving fewer moving block events. Probably not a critical issue but before approving the PR I'd like to double-check this with you @geriux.

Comment on lines 67 to 110
/**
* Helper function to track block movement events.
*
* @param {string} action Type of movement (arrows/drag & drop)
* @return {void}
*/
function trackBlockMoved( action ) {
const eventProperties = {
action_source: action,
};

sendEventToHost( 'editor_block_moved', eventProperties );
}
Copy link
Contributor

Choose a reason for hiding this comment

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

It would be interesting to collect more information like the type of block being moved or if it has inner blocks, similar to what we do in the above trackBlocksHandler function. For the purpose of measuring the drag & drop blocks feature this information is enough, so we could apply this suggestion in the future and create a follow-up issue for this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@geriux
Copy link
Contributor Author

geriux commented May 10, 2022

However, I noticed that moving a block by long-pressing over the up/down arrows and tapping on "Move to Top" or "Move to bottom" is not generating an event. Not sure if these options are heavily used by users, but it might lead to receiving fewer moving block events. Probably not a critical issue but before approving the PR I'd like to double-check this with you

Nice catch! I think we should track those as well, should we include them in the action move_arrows or maybe it'd be interesting to track them separately? like move_arrows_to_top, move_arrows_to_bottom. What do you think? @fluiddot

@fluiddot
Copy link
Contributor

However, I noticed that moving a block by long-pressing over the up/down arrows and tapping on "Move to Top" or "Move to bottom" is not generating an event. Not sure if these options are heavily used by users, but it might lead to receiving fewer moving block events. Probably not a critical issue but before approving the PR I'd like to double-check this with you

Nice catch! I think we should track those as well, should we include them in the action move_arrows or maybe it'd be interesting to track them separately? like move_arrows_to_top, move_arrows_to_bottom. What do you think? @fluiddot

Good question, I'd lean towards the option that collects more information (i.e. tracking them separately). However, this makes me think if we should track the up and down movements separately too, wdyt?

Potential sources of block movement action:

  • drag_and_drop
  • move_arrows_up
  • move_arrows_down
  • move_arrows_to_top
  • move_arrows_to_bottom

@geriux
Copy link
Contributor Author

geriux commented May 10, 2022

However, this makes me think if we should track the up and down movements separately too, wdyt?

I agree! I'll update them. Thank you!

@geriux
Copy link
Contributor Author

geriux commented May 10, 2022

Hey @fluiddot 👋 this is ready for another review 😃, the Android build is ready. The iOS build is still in progress.

@geriux geriux requested a review from fluiddot May 10, 2022 12:29
@geriux
Copy link
Contributor Author

geriux commented May 10, 2022

The iOS build is still in progress

Unfortunately, there are some issues with the CI job that creates an installable build, since it might take a while, would you be able to test it using a local build?

@fluiddot
Copy link
Contributor

The iOS build is still in progress

Unfortunately, there are some issues with the CI job that creates an installable build, since it might take a while, would you be able to test it using a local build?

Sure thing, I'll test it locally 👍.

@geriux geriux modified the milestones: 1.76.0, 1.75.1 (19.8) May 10, 2022
Copy link
Contributor

@fluiddot fluiddot left a comment

Choose a reason for hiding this comment

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

I tested the latest changes and look great to me, however, I noticed that the drag_and_drop event is being only sent in some cases when dragging and dropping the block in its original location (see attached video capture). Upon further investigation, I see that depending on if the block is dropped before and after, it sends the event:

  • Dropped before - Doesn't send the event.
  • Dropped after - Sends the event.
ios-drag-and-drop-track-events.mp4

src/analytics/redux/tracked_events.js Show resolved Hide resolved
src/analytics/redux/tracked_events.js Outdated Show resolved Hide resolved
src/analytics/redux/tracked_events.js Outdated Show resolved Hide resolved
@geriux
Copy link
Contributor Author

geriux commented May 10, 2022

Upon further investigation, I see that depending on if the block is dropped before and after, it sends the event:

Thanks for spotting the issue, when dragging the block right above and staying in the same position doesn't call moveBlocksToPosition when dropping because the index value is the same, when dragging right after it does since the index value is currentIndex + 1.

I feel it'd be an issue if we want to know if drag & drop was enabled in all cases, regardless if a block was actually moved. With the current approach, it will only be tracked if the block was actually moved. I think in this case, a "bug" would make sense but let me know if we should still track it anyways 😄

@geriux geriux requested a review from fluiddot May 10, 2022 15:20
@fluiddot
Copy link
Contributor

Upon further investigation, I see that depending on if the block is dropped before and after, it sends the event:

Thanks for spotting the issue, when dragging the block right above and staying in the same position doesn't call moveBlocksToPosition when dropping because the index value is the same, when dragging right after it does since the index value is currentIndex + 1.

Ok, I see, so in this case, the block is actually being moved but to the same origin position.

I feel it'd be an issue if we want to know if drag & drop was enabled in all cases, regardless if a block was actually moved. With the current approach, it will only be tracked if the block was actually moved. I think in this case, a "bug" would make sense but let me know if we should still track it anyways 😄

Right, probably the issue here is more related to the fact that the block is being moved than actually sending the event. Hence, the event logic is behaving as expected.

Regarding the issue related to preventing the block to be moved if the target location is the same as the origin, I think we could open a new issue as a follow-up.

@geriux
Copy link
Contributor Author

geriux commented May 11, 2022

Regarding the issue related to preventing the block to be moved if the target location is the same as the origin, I think we could open a new issue as a follow-up.

Well, currently it is intended to not move a block if it's in the same position, as you can see here.

@geriux geriux requested a review from fluiddot May 11, 2022 07:24
Copy link
Contributor

@fluiddot fluiddot left a comment

Choose a reason for hiding this comment

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

LGTM 🎊 ! Awesome work @geriux 🙇 !

NOTE: Following this note from the PR's description, before merging we should revert the Gutenberg reference update and JS bundle.

The bundle and Gutenberg reference changes are included to be able to test the drag & drop event, those will not be included once this is approved and ready to be merged.

@geriux geriux force-pushed the feature/drag-and-drop-analytics branch from 64337fa to 1d6316a Compare May 11, 2022 08:21
@geriux geriux merged commit ce75ce1 into trunk May 11, 2022
@geriux geriux deleted the feature/drag-and-drop-analytics branch May 11, 2022 08:47
@geriux geriux mentioned this pull request May 11, 2022
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Drag & Drop blocks] Track block movement actions
2 participants