Skip to content
This repository has been archived by the owner on Feb 23, 2024. It is now read-only.

Migration of Product Collection and Product Button blocks to the new store() API #11558

Conversation

luisherranz
Copy link
Collaborator

@luisherranz luisherranz commented Nov 2, 2023

What

Migrates both the Product Collection and the Product Button block to the new store() API.

Resources:

Why

Because we are switching from the old store() API to the new store() API, and we need to migrate all the blocks at once.

Testing Instructions

  1. Load a template that contains with Product Button blocks
  2. Test that it keeps working as exactly as it is working on trunk

Screenshots or screencast

I made a video to explain the migration of this block:

https://www.loom.com/share/cbe30d0c820e4f99a3e6697a5f85b759

WooCommerce Visibility

Required:

  • WooCommerce Core
  • Feature plugin
  • Experimental
  • N/A

Checklist

Required:

  • This PR has either a [type] label or a [skip-changelog] label.
  • This PR is assigned to a milestone.

Conditional:

  • This PR has a changelog description (if [skip-changelog] label is not present).
  • This PR adds/removes a feature flag & I've updated this doc.
  • This PR adds/removes an experimental interfaces, and I've updated this doc.
  • This PR has been accessibility tested.
  • This PR has had any necessary documentation added/updated.

Changelog

Add suggested changelog entry here.

Migrate the Product Button to the new store() API of the Interactivity API.

luisherranz and others added 30 commits September 26, 2023 10:52
Because the animation was broken.
@github-actions github-actions bot added this to the 11.6.0 milestone Nov 10, 2023
Copy link
Contributor

@Aljullu Aljullu left a comment

Choose a reason for hiding this comment

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

I really like how the code gets much simpler with these changes, good job @luisherranz!

The code changes look good to me and I did some smoke testing and everything is working as expected. However, I don't have much experience in how the Interactivity API worked before, so I would also like if @gigitux can take a look and give his approvall.

By the way, before merging there will need to be some changes to the PR itself: WooCommerce Visibility should be WooCommerce Core and we should add a type label (I guess type: enhancement?).

} );
return getTextButton(
context.addToCartText,
state.inTheCartText!,
Copy link
Contributor

Choose a reason for hiding this comment

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

Heads-up that there is an ESLint warning in this line and L121.

Copy link
Contributor

@gigitux gigitux left a comment

Choose a reason for hiding this comment

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

Thanks for your precious work and the helpful video!

I just added a NIT, feel free to skip it! However, I noticed two things:

  • A Circular dependency is detected:
image
  • Currently, there is an E2E test that fails: "21 [blockTheme] › product-collection/product-collection.block_theme.spec.ts:29:2 › Product Collection › Renders product collection block correctly with 9 items (27.0s)". I already re-run it, but it failed again:

image

@@ -168,20 +164,27 @@ protected function render( $attributes, $content, $block ) {
$this->prevent_cache();
}

$div_directives = 'data-wc-context=\'' . wp_json_encode( $context, JSON_NUMERIC_CHECK ) . '\'';
$interactive = array(
'namespace' => 'woocommerce/product-button',
Copy link
Contributor

Choose a reason for hiding this comment

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

NIT: We could use using the variables to compose the namespace:

Suggested change
'namespace' => 'woocommerce/product-button',
'namespace' => $this->namespace . "/" . $this->block_name,

@luisherranz
Copy link
Collaborator Author

A Circular dependency is detected

We can turn CHECK_CIRCULAR_DEPS off in the Interactivity API config.

@gigitux
Copy link
Contributor

gigitux commented Nov 13, 2023

A Circular dependency is detected

We can turn CHECK_CIRCULAR_DEPS off in the Interactivity API config.

I didn't check the code, but circular dependency could introduce some issues that could be hard to find/debug. I would be more oriented to fix the circular dependency, but it is just an opinion based on my past experience.

@luisherranz
Copy link
Collaborator Author

luisherranz commented Nov 13, 2023

Oh, sorry, I didn't explain myself well enough. I wouldn't suggest changing how you manage your code. But that warning comes from the Interactivity API runtime, so it's ok to remove that circle dep check in the specific Webpack config of the Interactivity API runtime. It won't affect any Woo code.

@gigitux
Copy link
Contributor

gigitux commented Nov 13, 2023

Oh, sorry, I didn't explain myself well enough. I wouldn't suggest changing how you manage your code. But that warning comes from the Interactivity API runtime, so it's ok to remove that circle dep check in the specific Webpack config of the Interactivity API runtime. It won't affect any Woo code.

Yeah, I got it!
My suggestion was to fix it, even if it is in the Interactivity API runtime (not a blocker for this PR). But as I said, it is just an opinion that I have on circular dependency based on past experience :D

@DAreRodz DAreRodz added the type: enhancement The issue is a request for an enhancement. label Nov 13, 2023
@luisherranz
Copy link
Collaborator Author

Thanks, Luigi. For now, we're trying to make things work and want to keep iterating fast. We'll make them pretty after we move them to Gutenberg and we are about to merge them with WP Core 🙂

@DAreRodz
Copy link
Collaborator

By the way, before merging there will need to be some changes to the PR itself: WooCommerce Visibility should be WooCommerce Core and we should add a type label (I guess type: enhancement?).

I've updated the visibility and the type label. Feel free to modify them as needed. 🙂

BTW, this branch uses another PR as the base branch, but I added the changes just in case we decide to merge the base branch first to trunk.

@gigitux
Copy link
Contributor

gigitux commented Nov 13, 2023

With 1a3d9ca I fixed the E2E test failure. For more details, check this issue (we should use WP_HTML_Tag_Processor). The other E2E tests fail due to the Product Gallery block and it is outside of the scope of this PR.

Thanks, Luigi. For now, we're trying to make things work and want to keep iterating fast. We'll make them pretty after we move them to Gutenberg and we are about to merge them with WP Core

Thanks for the clarification. It makes sense 👍

Copy link
Contributor

@gigitux gigitux left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks for your work!

luisherranz and others added 2 commits November 16, 2023 15:54
* Migrate Product Gallery block to new Interactivity API store

* Fix some references

* Add missing data-wc-interactive

* Fix an additional namespace

* Remove unnecessary click handler

* Dialog working

* Refactor action names

* Reindex PHP array

There was some missing indexes, which turned the array into an object in JS.

* Remove unused event handlers

* Move next/previous logic to external function

* Move StorePart util to the types folder

* Rename namespace to `woocommerce/product-gallery`

* Undo product collection namespace renaming

* Remove unnecessary namespace

* Don't hide the large image on page load

* Minor refactorings

* Fix eslint error

* Fix php cs errors with spacing and double arrows alignment

* Disable no-use-before-define rule for eslint

* Disable @typescript-eslint/ban-types rule for eslint

* Fix parsed context error in e2e tests

* Fix context parser for Thumbnail image

* Move store to the top of the frontend file

* Add interactivity api utils to the @woocommerce/utils alias

* Replace deprecated event attribute

---------

Co-authored-by: Luis Herranz <luisherranz@gmail.com>
Co-authored-by: David Arenas <david.arenas@automattic.com>
Co-authored-by: roykho <roykho77@gmail.com>
Copy link
Contributor

Replace with an interactive block that calls `actions.sel...

Replace with an interactive block that calls `actions.selectImage`.


// TODO: Replace with an interactive block that calls `actions.selectImage`.
const observer = new MutationObserver( function ( mutations ) {
for ( const mutation of mutations ) {
const mutationTarget = mutation.target as HTMLElement;
const currentImageAttribute =
mutationTarget.getAttribute( 'current-image' );
if (
mutation.type === 'attributes' &&
currentImageAttribute &&
context.visibleImagesIds.includes(
currentImageAttribute
)

🚀 This comment was generated by the automations bot based on a todo comment in c10c777 in #11558. cc @luisherranz

@DAreRodz DAreRodz merged commit 9ea4bfe into interactivity-api-new-store-api Nov 16, 2023
28 of 29 checks passed
@DAreRodz DAreRodz deleted the interactivity-api-refactor-product-button-with-new-store-api branch November 16, 2023 22:45
@DAreRodz
Copy link
Collaborator

I've merged it even though some tests did not pass. 🤔

To be honest, I'm wondering if they were failing because of a real issue in the code, because everything was green in #11721 and that branch already contained all the changes from this one (#11558), so they should be equivalent once it was merged here.

DAreRodz added a commit that referenced this pull request Nov 21, 2023
* Sync Interactivity API code with Gutenberg

* New store() API

* Store raw actions

* Update wc-interactivity-store implementation

* Replace `wc_store` with `wc_initial_state`

* Parse and populate initial state

* Allow store parts in `store()`

* Accept namespaces in directive paths

* Add $$namespace to directives' object values

* Make namespace parsing more robust

* Use DeepPartial type for store parts

* Do not pass `rawStore` to `afterLoad` callbacks

* Simplify `store()` a bit

* Implement `privateStore()`

* Sync context directive with Gutenberg

* Refactor scope and extract getters per scope

* Add namespace to getters and actions

* Remove current privateStore implementation

* Remove `afterLoad` option from `store`

* Use same proxy handlers for ns, getters and actions

* Set scope inside `evaluate`

* Refactor proxy handlers

* Improve types a bit

* Catch errors in async actions

* Implement stacks for scopes and namespaces

* Implement `getElement`

* Change directives object structure

* Remove unnecessary import

* Implement private stores

* Return value from sync actions

* Minor optimizations and improved comments

* Don't use async inside `data-wp-watch`

* Use a single Provider in context directive

* Remove DeepPartial type

* Do not check if element exists

* Add the `current` prop of state inside the scope

* Move getters outside scope

* Fix wc-key assignment

* Fix missing `navigate` in directives

* Fix namespace not being picked in the same element

* Deep merge raw stores instead of proxied ones

* Fix namespace assignment

* Allow forward slashes in namespaces

* Migration of Product Collection and Product Button blocks to the new `store()` API (#11558)

* Refactor Product Button with new store() API

* Use `wc_initial_state` in Product Button

* Fix namespace

* Remove unnecessary state

* Test namespaces in directive paths

* Add test context with namespace

* Simplify woo-test context

* Move addToCart and animations to a file

* Do not pass `rawStore` to `afterLoad` callbacks

* Move callbacks and actions back to the main file

Because the animation was broken.

* Remove selectors in favor of state

* Use default ns in `getContext` for state and actions

* Remove `afterLoad` callback

* Remove unnecessary ns

* Fix getContext in add-to-cart

* Replace namespace and delete unnecessary store

* Pass context types only once

* Use an alternative for requestIdleCallback

* Add previous react code for notices

* Add namespace to Product Collection block

* Replace getTextButton with getButtonText

* Add block name to the ProductCollection namespace

* fix style HTML code

* Remove circular deps error on the Interactivity API

* Product Gallery block: Migrate to new Interactivity API store (#11721)

* Migrate Product Gallery block to new Interactivity API store

* Fix some references

* Add missing data-wc-interactive

* Fix an additional namespace

* Remove unnecessary click handler

* Dialog working

* Refactor action names

* Reindex PHP array

There was some missing indexes, which turned the array into an object in JS.

* Remove unused event handlers

* Move next/previous logic to external function

* Move StorePart util to the types folder

* Rename namespace to `woocommerce/product-gallery`

* Undo product collection namespace renaming

* Remove unnecessary namespace

* Don't hide the large image on page load

* Minor refactorings

* Fix eslint error

* Fix php cs errors with spacing and double arrows alignment

* Disable no-use-before-define rule for eslint

* Disable @typescript-eslint/ban-types rule for eslint

* Fix parsed context error in e2e tests

* Fix context parser for Thumbnail image

* Move store to the top of the frontend file

* Add interactivity api utils to the @woocommerce/utils alias

* Replace deprecated event attribute

---------

Co-authored-by: Luis Herranz <luisherranz@gmail.com>
Co-authored-by: David Arenas <david.arenas@automattic.com>
Co-authored-by: roykho <roykho77@gmail.com>

---------

Co-authored-by: David Arenas <david.arenas@automattic.com>
Co-authored-by: Luigi Teschio <gigitux@gmail.com>
Co-authored-by: Alexandre Lara <allexandrelara@gmail.com>
Co-authored-by: roykho <roykho77@gmail.com>

* Fix error when closing product gallery dialog with keyboard escape key

* use wc_initial_state instead of wc_store

---------

Co-authored-by: Luis Herranz <luisherranz@gmail.com>
Co-authored-by: Luigi Teschio <gigitux@gmail.com>
Co-authored-by: Alexandre Lara <allexandrelara@gmail.com>
Co-authored-by: roykho <roykho77@gmail.com>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
type: enhancement The issue is a request for an enhancement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants