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

Insert Block command #59

Merged
merged 9 commits into from
May 4, 2022
Merged

Insert Block command #59

merged 9 commits into from
May 4, 2022

Conversation

cadic
Copy link
Contributor

@cadic cadic commented Apr 20, 2022

Description of the Change

Adds insertBlock command. Yields the block element ID:

cy.insertBlock('core/heading').then(id => {
  cy.get(`#${id}`).click().type(heading);
});

Tests include custom block from 10up/retro-winamp-block, it doesn't support WordPress 5.2, so this test is optional.

Checklist:

  • I have read the CONTRIBUTING document.
  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have added tests to cover my change.
  • All new and existing tests passed.

Changelog Entry

Added - insertBlock command

Credits

Props @cadic

@cadic cadic self-assigned this Apr 20, 2022
@cadic
Copy link
Contributor Author

cadic commented Apr 21, 2022

@dinhtungdu before this PR goes to review I have question:

This PR adds "Insert Block" command and is tested against different built-in block types, including sub-blocks (core/embed/twitter and core/post-navigation-link/post-next). It will be also great to include test of a custom block, but in this case our example WordPress plugin will grow and definitely needs to be moved to a directory. I think this is required because custom blocks are handled a bit different in the core (f.e. the selectors are different).

Do you think it worth to be done or we could keep core-only blocks in our test?

@dinhtungdu
Copy link
Contributor

It will be also great to include test of a custom block, but in this case our example WordPress plugin will grow and definitely needs to be moved to a directory.

@cadic This is a blocker to me as well.

Do you think it worth to be done or we could keep core-only blocks in our test?

I think as long as we pass the correct block name, there is no difference between core and custom blocks. So my answer to your question is no, I don't think it's worth adding a custom block to our example plugin.

On the other hand, we can test inserting custom blocks without creating our own. I think any simple block plugin in the block directory can be used for our testing.

@cadic
Copy link
Contributor Author

cadic commented Apr 22, 2022

@dinhtungdu thank you for the idea of using external plugin for custom blocks. Added https://github.com/10up/retro-winamp-block/ and it works like a charm!

@cadic cadic requested a review from dinhtungdu April 22, 2022 08:47
Copy link
Contributor

@dinhtungdu dinhtungdu left a comment

Choose a reason for hiding this comment

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

Overall, this is LGTM! But I have an issue with the custom block test for WP 5.9.3: the custom block test was skipped. The Winamp block was there but wasn't inserted into the editor:
Screen Shot 2022-04-27 at 09 30 00

@cadic
Copy link
Contributor Author

cadic commented Apr 27, 2022

@dinhtungdu thank you so much for checking the tests on your side. The command itself is fine, but the test has a typo in its conditional part which caused skipping the test when it should be executed.

Copy link
Contributor

@dinhtungdu dinhtungdu left a comment

Choose a reason for hiding this comment

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

LGTM 🚢

@cadic cadic merged commit 5273358 into trunk May 4, 2022
@cadic cadic deleted the command/insert-block branch May 4, 2022 19:19
github-actions bot pushed a commit that referenced this pull request May 4, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants