-
Notifications
You must be signed in to change notification settings - Fork 312
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
Support transforming legacy widgets to blocks and hide widgets when the blocks are available #2819
Conversation
I've finally added complete tests for the Related Posts block, but the Facets block still needs new tests as the current tests are based on using the Legacy Widget as a block. |
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.
Overall it looks really good @JakePT. Left some comments about specific items.
I'd like to know your thoughts about testing the "transform" part of those blocks. Isn't that worth testing in Cypress?
/** | ||
* Test that the Related Posts block is functional. | ||
*/ | ||
it('Facet block', () => { |
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.
@JakePT do you mind rephrasing this to describe what is being tested?
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.
@felipeelia Done.
* Test that the Facet widget is functional and can be transformed into the | ||
* Facet block. | ||
*/ | ||
it('Facet widget', () => { |
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.
@JakePT same here
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.
@felipeelia Done.
/** | ||
* Visit the block-based widgets screen. | ||
*/ | ||
cy.wpCli('wp plugin deactivate classic-widgets'); |
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.
@JakePT we have a command to activate and deactivate plugins. I'd prefer to stick with that just to be sure any performance improvement we have is applied across the entire test suite.
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.
@felipeelia Done.
cy.wpCli('elasticpress index --setup --yes'); | ||
cy.wpCli('plugin install classic-widgets'); |
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.
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.
@felipeelia Done.
tests/cypress/support/commands.js
Outdated
Cypress.Commands.add('openBlockSettingsSidebar', () => { | ||
cy.get('body').then(($el) => { | ||
if ($el.hasClass('widgets-php')) { | ||
cy.get('.edit-widgets-header__actions button[aria-label="Settings"]').click(); | ||
cy.get('.edit-widgets-sidebar__panel-tab').contains('Block').click(); | ||
} else { | ||
cy.get('.edit-post-header__settings button[aria-label="Settings"]').click(); | ||
cy.get('.edit-post-sidebar__panel-tab').contains('Block').click(); | ||
} | ||
}); | ||
}); | ||
|
||
Cypress.Commands.add('openBlockInserter', () => { | ||
cy.get('body').then(($el) => { | ||
if ($el.hasClass('widgets-php')) { | ||
cy.get('.edit-widgets-header-toolbar__inserter-toggle').click(); | ||
} else { | ||
cy.get('.edit-post-header-toolbar__inserter-toggle').click(); | ||
} | ||
}); | ||
}); | ||
|
||
Cypress.Commands.add('getBlocksList', () => { | ||
cy.get('.block-editor-inserter__block-list'); | ||
}); | ||
|
||
Cypress.Commands.add('insertBlock', (blockName) => { | ||
cy.get('.block-editor-block-types-list__item').contains(blockName).click(); | ||
}); |
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 wonder if we shouldn't start a new file only with blocks. Thoughts?
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.
@felipeelia Done.
Description of the Change
This PR does two things:
Both changes are implemented as per the guidance in the Block Editor Handbook and the behaviour is consistent with how most core legacy widgets are handled, such as Search.
Closes #2792
Possible Drawbacks
While existing block instances won't be affected, the user will not be able to add new instances of the legacy widget.
However, if the user doesn't want to switch existing instances to the block and needs to add new legacy widget instances, then there are two workarounds:
widget_types_to_hide_from_legacy_widget_block
filter hook to remove their IDs from the list filtered by the hook.Verification Process
Checklist:
Changelog Entry
Added - Support for transforming instances of the legacy Facet and Related Posts widgets into blocks.
Changed - The legacy Facet and Related Posts widgets are now hidden when using the block editor.
Credits
Props @JakePT