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

Drag & Drop blocks feature #4832

Merged
merged 14 commits into from
May 11, 2022
Merged

Drag & Drop blocks feature #4832

merged 14 commits into from
May 11, 2022

Conversation

geriux
Copy link
Contributor

@geriux geriux commented May 5, 2022

What?

Introduces the drag & drop blocks feature to the native version of the editor.

Why?

This is the integration PR of the drag & drop blocks feature.

How?

The implementation details can be found at WordPress/gutenberg#40424.

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 10, 2022

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

@fluiddot fluiddot added this to the 1.76.0 milestone May 10, 2022
@fluiddot fluiddot marked this pull request as ready for review May 10, 2022 16:28
@fluiddot fluiddot self-requested a review May 10, 2022 16:29
Copy link
Contributor Author

@geriux geriux left a comment

Choose a reason for hiding this comment

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

LGTM! I can't approve the PR but all good from my side ✅

@fluiddot
Copy link
Contributor

fluiddot commented May 11, 2022

@geriux looks like the E2E test failures are actual breakages 😭. I investigated further and at least on Android, it's related to the fact that the block's content is now wrapped with a View component. The text block queries look for an EditText children element of the block, which can't be found because of the wrapper View. It should be relatively easy to address the issue just by updating the queries so the EditText element could be found at any nested level.

Example

Current implementation

  • XPath query: //android.view.ViewGroup[contains(@content-desc, "Paragraph Block. Row 2.")]/android.widget.EditText

EditText element is expected to be children of the block but at the first level.

Future implementation

  • XPath query: //android.view.ViewGroup[contains(@content-desc, "Paragraph Block. Row 2.")]//android.widget.EditText (Note the change: / => //)

EditText element is expected to be children of the block but at any nested level.


I will work on applying this fix in the Gutenberg PR and check if the fix makes the CI jobs pass 🤞 .

@geriux
Copy link
Contributor Author

geriux commented May 11, 2022

I will work on applying this fix in the Gutenberg PR and check if the fix makes the CI jobs pass 🤞 .

Cool! Thanks for explaining the issue and fixing it! It looks like they passed now 🎉

@geriux
Copy link
Contributor Author

geriux commented May 11, 2022

Should we maybe run the full ones just to double check?

@fluiddot
Copy link
Contributor

Should we maybe run the full ones just to double check?

Yeah, I'm going to trigger the full ones because there was one on iOS that was failing.

Screenshot 2022-05-11 at 14 21 41

@fluiddot
Copy link
Contributor

Regarding the failure of the iOS E2E test, I applied the fix in WordPress/gutenberg@7292638. I'm going to update the Gutenberg reference and trigger all PR checks again.

@fluiddot
Copy link
Contributor

I will work on applying this fix in the Gutenberg PR and check if the fix makes the CI jobs pass 🤞 .

Cool! Thanks for explaining the issue and fixing it! It looks like they passed now 🎉

Yeah 🎊 ! Looks like the fix (WordPress/gutenberg@decf119) for the Android E2E tests worked.

@fluiddot
Copy link
Contributor

@geriux Finally all PR checks have passed 🎊 , I'll proceed to merge the PRs.

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.

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.

2 participants