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

[Product Collection] POC: Preview Mode using HOC #45703

Closed
wants to merge 21 commits into from

Conversation

imanish003
Copy link
Contributor

@imanish003 imanish003 commented Mar 19, 2024

Changes proposed in this Pull Request:

We are working on Preview mode for Product Collection block. Here's the scoop: some collections might look a bit different in the Editor than they do on the Frontend. Take the Recently Viewed collection as an example; its appearance in the Editor might not exactly mirror its Frontend display. To bridge this gap, we're thinking of introducing a label, which you can peek at in the screenshot below.

Here's what we're aiming for:

  • We want all collections, including those from third parties, to have the flexibility to toggle this preview label on or off as needed.
  • In scenarios where a collection is processing something in the background, it can signal that the Editor UI is currently showing a preview. Once the background work wraps up, it can update the status to show the Editor UI is no longer in preview mode.
  • When someone hovers over the preview label, a tooltip should pop up. This tooltip could vary from one collection to another, and collections should have the freedom to customize this tooltip message. Check out the screenshot below for a glimpse:

Tooltip

Different tooltip messages

Proposed solution in this POC

  • I have abstracted logic to register a new collection into registerProductCollection. This function will allow us to register a new collection. For now, it's not public, but later we plan to make it public (i.e. usable by 3PDs). For now, this function accepts all the arguments which are accepted by Block Variation

  • registerProductCollection will accept an argument handlePreviewState. Here is how it will look like

registerProductCollection({
	name: 'my-custom-collection',
	title: 'My Custom Collection',
	// Other necessary properties for the collection...
	handlePreviewState: ({ setPreviewState }) => {
	  setPreviewState({
	    isPreview: true,
	    previewMessage: 'This is a preview of My Custom Collection.',
	  });
	},
});
  • As you can see, Collection will have access to setPreviewState in handlePreviewState, which will allow them to set the preview state. It can be used to show/hide the preview label and also set tooltip message.

Need Feedback

I'd love for you to take a moment to review the updates in this PR and share your invaluable feedback with me. 🙂

Closes #45822

How to test the changes in this Pull Request:

Using the WooCommerce Testing Instructions Guide, include your detailed testing instructions:

Changelog entry

  • Automatically create a changelog entry from the details below.

Significance

  • Patch
  • Minor
  • Major

Type

  • Fix - Fixes an existing bug
  • Add - Adds functionality
  • Update - Update existing functionality
  • Dev - Development related task
  • Tweak - A minor adjustment to the codebase
  • Performance - Address performance issues
  • Enhancement - Improvement to existing functionality

Message

Comment

POC: Implement preview mode for Product Collection block in editor
- Added extensive commentary to clarify the mechanism and usage of the `handlePreviewState` function
- Implemented an internal state within `ProductCollectionContent` to manage preview status and messages, serving as a foundational example of how preview mode can enrich block functionality.
- Showcased the application of `handlePreviewState` by incorporating it as a prop in `BlockEdit`, illustrating the potential for extending the block's capabilities for dynamic and interactive previews.

This POC demonstrates a flexible approach to managing preview states within the editor, paving the way for further development and integration based on feedback and use-case analysis.
Copy link
Contributor

github-actions bot commented Mar 19, 2024

Test Results Summary

Commit SHA: 7950dfd

Test 🧪Passed ✅Failed 🚨Broken 🚧Skipped ⏭️Unknown ❔Total 📊Duration ⏱️
API Tests25900202610m 38s
E2E Tests00036303630m 0s

To view the full API test report, click here.
To view the full E2E test report, click here.
To view all test reports, visit the WooCommerce Test Reports Dashboard.


return (
// Here we are passing the handlePreviewState as a prop to the BlockEdit.
<BlockEdit { ...props } handlePreviewState={ handlePreviewState } />
Copy link
Contributor

Choose a reason for hiding this comment

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

Question:

do you really need to have a function passed in to change the state? React has props already for this purpose.

If you want to update the component every 5 seconds then you just handle the state here in the wrapper. I think thats less complicated than requiring users to pass a function to do the state update.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's a good question, and I appreciate you bringing it up! In light of your query, I've made a few adjustments to this PR. As outlined in the PR description, I introduced a function called registerProductCollection which is pivotal for every collection to seamlessly register itself. While it's currently for internal use, our vision is to eventually open it up for 3PDs to utilize. This function is designed to be compatible with all parameters supported by Block Variation.

Following your question, and as detailed in the recent updates, our aim is to avoid necessitating collections to construct a wrapper around 'editor.BlockEdit'. This responsibility is efficiently handled by the registerProductCollection function, thereby necessitating the need for a callback function to manage such integrations.

I hope this clarifies the rationale behind these changes! Please let me know if I am missing something 🙂

Comment on lines 261 to 279
const handlePreviewState = (
previewState: PreviewState,
setPreviewState: React.Dispatch<
React.SetStateAction< PreviewState >
>
) => {
setTimeout( () => {
setPreviewState( {
isPreview: ! previewState.isPreview,
previewMessage,
} );
}, 5000 );
};

return (
// Here we are passing the handlePreviewState as a prop to the BlockEdit.
<BlockEdit { ...props } handlePreviewState={ handlePreviewState } />
);
};
Copy link
Contributor

Choose a reason for hiding this comment

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

If you just use props this would become:

Suggested change
const handlePreviewState = (
previewState: PreviewState,
setPreviewState: React.Dispatch<
React.SetStateAction< PreviewState >
>
) => {
setTimeout( () => {
setPreviewState( {
isPreview: ! previewState.isPreview,
previewMessage,
} );
}, 5000 );
};
return (
// Here we are passing the handlePreviewState as a prop to the BlockEdit.
<BlockEdit { ...props } handlePreviewState={ handlePreviewState } />
);
};
const [preview, setPreview] = useState(true);
const previewMessage = 'whatever';
useEffect(() => {
const timeoutId = setTimeout(() => {
setPreview(!preview);
}, 5000);
return () => clearTimeout(timeoutId);
}, []);
return (
<BlockEdit { ...props } previewMessage={previewMessage} isPreview={preview} />
);
};

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just left a comment at #45703 (comment). Please let me know if you have any follow-up questions. 🙂

This commit introduces a centralized approach for registering product collection variations and managing their preview states. It abstracts the registration logic into a dedicated function and enhances the flexibility of preview state handling across different collection types.
I don't see any good use of it in handlePreviewState. Also, We will be going to call handlePreviewState only once
therefore, it will always have the same value as the initial value of the previewState. If in future, we decide to run it
multiple times then we can pass the previewState as an argument to handlePreviewState.
This commit introduces a refined approach for injecting the `handlePreviewState` function into product collection blocks, utilizing JavaScript closures to streamline the process. This method replaces the previous global registry mechanism, offering a more direct and efficient way to manage preview states.

Advantages of This Approach:
- Utilizing JavaScript closures for injecting `handlePreviewState` simplifies the overall architecture by directly modifying block edit components without relying on an external registry. This method enhances code clarity and reduces the cognitive load for developers.
- The conditional application of `withHandlePreviewState` ensures that the preview state handling logic is only added to blocks that require it, optimizing performance and maintainability.
This commit enhances the organization and readability of the product collection content component by abstracting the preview state management into a custom hook named `usePreviewState`. This change streamlines the component's structure and aligns with React best practices for managing state and side effects.

Key Changes:
- Introduced `usePreviewState`, a custom hook responsible for initializing and managing the preview state (`isPreview` and `previewMessage`) of the product collection block. This hook encapsulates the state logic and its side effects, including the conditional invocation of `handlePreviewState`.
- Modified `ProductCollectionContent` to utilize `usePreviewState` for handling its preview state. This adjustment makes the component cleaner and focuses it more on presentation and behavior rather than state management details.
@imanish003
Copy link
Contributor Author

imanish003 commented Mar 22, 2024

Hey @kmanijak @xristos3490 @samueljseay @ObliviousHarmony 👋

This POC is ready for review. I'd appreciate it if you could take a moment to review the updates in this PR and provide your valuable feedback. Please take a look at this PR description for more info 🙂

Copy link
Contributor

github-actions bot commented Mar 22, 2024

Hi @xristos3490, @samueljseay, @ObliviousHarmony, @kmanijak,

Apart from reviewing the code changes, please make sure to review the testing instructions as well.

You can follow this guide to find out what good testing instructions should look like:
https://github.com/woocommerce/woocommerce/wiki/Writing-high-quality-testing-instructions

* any async operation like fetching data from server or doing a polling in every 5 seconds.
*/
const handlePreviewState = ( { setPreviewState }: HandlePreviewStateArgs ) => {
setTimeout( () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Assuming long polling like this is a use-case you want to support it might make sense to also support clean up as a return value.

In this function you'd return a function that does the clean up, which you can then return from the useEffect cleanup.

e.g. at the end of this function:

return () => {
  clearTimeout(timeoutId);
 }

then in the component:

	useEffect( () => {
   	const cleanup = handlePreviewState?.( {
   		setPreviewState,
   	} );
       if (cleanup) { return cleanup; }
   }, [] );

Copy link
Contributor

Choose a reason for hiding this comment

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

💯 !

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Great point! I will add a cleanup functionality.


// We want this to run only once, adding deps will cause performance issues.
// eslint-disable-next-line react-hooks/exhaustive-deps
}, [] );
Copy link
Contributor

Choose a reason for hiding this comment

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

Lets say I wrote some async handler, that starts with preview on and then switches it off, if I do that there will be a flash with this approach, we render the component with no preview and then handler will turn preview on (after mount), and then lets say 500ms later it will turn off again.

I think the API needs to support setting the initial state to ensure this flash wouldn't happen. some would want to start with it on, and others would want to start with it off. I'm not sure how to make the api for that very nice and I don't have an immediate idea of how to support that nicely with useState, but I think the concern is relevant.

Copy link
Contributor

Choose a reason for hiding this comment

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

Initially, I agreed with you. But then I thought such API may be more complicated than simply educating user that such a use case requires initial setPreviewState like this:

const handlePreviewState = ( { setPreviewState }: HandlePreviewStateArgs ) => {
    // Set initial state
    setPreviewState( {
			isPreview: true,
			previewMessage: __(
				'I don't know the status so I set isPreview to true by default.',
				'woocommerce'
			),
		} );
    
    // Do the async operation to find out the actual status
	setTimeout( () => {
		setPreviewState( {
			isPreview: INFO_FROM_REQUEST,
			previewMessage: __(
				'Custom tooltip for on sale collection.',
				'woocommerce'
			),
		} );
	}, 5000 );
};

I think it's rather clean and simple and we keep a single access point.

Copy link
Contributor

Choose a reason for hiding this comment

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

@kmanijak that won't work, there will still be a flash because we don't call setPreviewState for the first time until the component first mounts..

Maybe its fast enough to be rendered not noticeable here though? but there will be an initial redundant re-render.

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe its fast enough to be rendered not noticeable here though? but there will be an initial redundant re-render.

Yes, it should be fast enough but I agree - it's not optimal. On the other hand, it's a tradeoff between optimization and potentially more complex API. Let's see if we have some idea of how to expose a way to set the initial value! Maybe it won't be too complex and I'm simply wrong about that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think on top of what @kmanijak shared in #45703 (comment) comment, we can use useLayoutEffect instead of useEffect. This way we can avoid the initial flash.

useEffect vs. useLayoutEffect

  • useEffect: Runs asynchronously after the DOM has been updated. If you're making changes to the DOM or triggering a re-render from useEffect, those changes will not be applied immediately but in the next tick, potentially leading to visible flashes or janky updates if there's a significant difference between the initial and subsequent states.
  • useLayoutEffect: Runs synchronously after DOM mutations but before the browser has painted the changes to the screen. This timing allows you to make adjustments to the DOM or execute logic based on the DOM state without the user seeing any intermediate states, effectively preventing visual flashes or flickers caused by updates.

@samueljseay What do you think? Am I missing something? 🙂

Copy link
Contributor

Choose a reason for hiding this comment

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

@imanish003 great idea, I hadn't considered using that. Yes I think since we this is not resource intensive and won't be called often that we can use useLayoutEffect (you have to be cautious since it blocks browser from painting). Good thinking I think this would work fine!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Awesome, I will make the changes 🙌🏻

Copy link
Member

@xristos3490 xristos3490 left a comment

Choose a reason for hiding this comment

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

Hey @imanish003, great work on this! I really like the idea of offloading responsibilities like long-polling outside the core block. However, I do have some concerns about the usage of the location context. Can this function access the location context, especially considering a future where the location might be manually selected?

Also, it might be worth considering making the location context a dependency of the preview state, just in case a collection block is moved within a Single Product block or other actions that will dynamically change the location context.

In terms of the general idea, it sounds great, but I have a few thoughts.

  • Would we need to set up a specific 3pd rest endpoint to handle e.g., long-polling for determining if a collection is in preview mode? (not that I don't like it, just sanity checking the thinking behind)
  • Additionally, we may need to handle a mock query to display random products when the preview state is true. What would preview mode mean in terms of handling the server-side rendering query? Should we omit running the query altogether?

Overall, great job on this! Looking forward to seeing this idea progress.

Copy link
Contributor

@kmanijak kmanijak left a comment

Choose a reason for hiding this comment

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

Nice work @imanish003!

I tested this and even played with some interval changes (mimicking long-polling). Really nice work!

Here are some of my thoughts we should keep in mind in scope of the POC:

  1. Data fetching:
    • If the isPreview is true we should display some random products. My understanding is that right now let's say "Recently viewed" which will always be in "preview" mode, will display no products with "Preview" label.
    • If the isPreview switched from true to false we should retrigger products fetch. Otherwise, even if conditions to display actual products are met, we'd still display some random preview.

Ideas:
- One way would be to remove the dynamic option completely and allow for providing information about the preview only once on the initial render. That means it would require a page reload from merchant to see if the preview is gone or not. That removes the initial state problem, simplifies the logic and also removes the complexity of re-triggering the query. Basically, the Product Collection would be either in preview state or not, nothing in between.
- Alternatively, assuming we want to stick to state, we can address data fetching iteratively and display some static placeholder first and only add logic around data fetching in next iterations:

image
  1. Required context
    Let me know if you consider it a part of the problem to solve or not but Collections will have some "internal" reasons to display preview that should "override" whatever handlePreviewState provides.
    Collection should be able to specify requiredContext or requiredLocation. In case it's not satisfied we should not even call handlePreviewState and set the preview ourselves:
setPreviewState({
    isPreview: true,
    previewMessage: 'Actual products will vary depending on the currently viewed page.'
});

I understand it's a POC so no worries but wanted to make sure you got it on your radar in general.

  1. handlePreviewState arguments

What arguments do you think should be passed to the handlePreviewState? I think we should provide location, and maybe query? @xristos3490 what arguments would you expect in this function so you can make the decision if the Product Collection is in preview mode or not?


// We want this to run only once, adding deps will cause performance issues.
// eslint-disable-next-line react-hooks/exhaustive-deps
}, [] );
Copy link
Contributor

Choose a reason for hiding this comment

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

Initially, I agreed with you. But then I thought such API may be more complicated than simply educating user that such a use case requires initial setPreviewState like this:

const handlePreviewState = ( { setPreviewState }: HandlePreviewStateArgs ) => {
    // Set initial state
    setPreviewState( {
			isPreview: true,
			previewMessage: __(
				'I don't know the status so I set isPreview to true by default.',
				'woocommerce'
			),
		} );
    
    // Do the async operation to find out the actual status
	setTimeout( () => {
		setPreviewState( {
			isPreview: INFO_FROM_REQUEST,
			previewMessage: __(
				'Custom tooltip for on sale collection.',
				'woocommerce'
			),
		} );
	}, 5000 );
};

I think it's rather clean and simple and we keep a single access point.

* any async operation like fetching data from server or doing a polling in every 5 seconds.
*/
const handlePreviewState = ( { setPreviewState }: HandlePreviewStateArgs ) => {
setTimeout( () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

💯 !

@imanish003
Copy link
Contributor Author

@xristos3490

However, I do have some concerns about the usage of the location context. Can this function access the location context, especially considering a future where the location might be manually selected?

AFAIU It shouldn’t have any problem accessing the location context. It should be able to use useGetLocation hook developed by @kmanijak.

Also, it might be worth considering making the location context a dependency of the preview state, just in case a collection block is moved within a Single Product block or other actions that will dynamically change the location context.

Definitely, We can consider it. Is it something you want to be shipped in 1st version of Preview Mode? If yes, Do you have any specific example in which it will be helpful to decide if the collection is in preview mode? I am not passing location context to handlePreviewState yet.

Would we need to set up a specific 3pd rest endpoint to handle e.g., long-polling for determining if a collection is in preview mode? (not that I don't like it, just sanity checking the thinking behind)

It will be up to 3PDs, they can use whatever they want to decide if the collection is in preview mode or not. As this will be just a normal Javascript callback function.

  • They may call any REST API
  • They may use attributes value - I haven’t passed attributes value to handlePreviewState yet but I think I should add it as this is something which will be needed to decide if collection is in preview mode or not. What do you think?

Additionally, we may need to handle a mock query to display random products when the preview state is true.

Good point! When in preview mode, we can pull some random products for display in Editor.

What would preview mode mean in terms of handling the server-side rendering query? Should we omit running the query altogether?

I am not sure what you mean here. Can you please elaborate it further? 🙂

@imanish003
Copy link
Contributor Author

@kmanijak

Data fetching:

  • If the isPreview is true we should display some random products. My understanding is that right now let's say "Recently viewed" which will always be in "preview" mode, will display no products with "Preview" label.
  • If the isPreview switched from true to false we should retrigger products fetch. Otherwise, even if conditions to display actual products are met, we'd still display some random preview.

Ideas:

  • One way would be to remove the dynamic option completely and allow for providing information about the preview only once on the initial render. That means it would require a page reload from merchant to see if the preview is gone or not. That removes the initial state problem, simplifies the logic and also removes the complexity of re-triggering the query. Basically, the Product Collection would be either in preview state or not, nothing in between.
  • Alternatively, assuming we want to stick to state, we can address data fetching iteratively and display some static placeholder first and only add logic around data fetching in next iterations:

Absolutely, displaying random products or a placeholder during the preview phase makes perfect sense. And yes, re-fetching products once we're no longer in preview mode is crucial.

As for the ideas, I'll need a moment to ponder over them and will circle back to you with my thoughts. 🙂

Required context
Let me know if you consider it a part of the problem to solve or not but Collections will have some "internal" reasons to display preview that should "override" whatever handlePreviewState provides.
Collection should be able to specify require dContext or requiredLocation. In case it's not satisfied we should not even call handlePreviewState and set the preview ourselves:

You've raised an excellent point! When we get to implementing this, I'm confident we can adjust the logic here so that handlePreviewState isn’t called unless requiredContext or requiredLocation conditions are fulfilled. Thank you for highlighting this; it's definitely an important consideration. 🙂

What arguments do you think should be passed to the handlePreviewState? I think we should provide location, and maybe query@xristos3490 what arguments would you expect in this function so you can make the decision if the Product Collection is in preview mode or not?

Good question! I think we can provide location and all attributes (including query ) of Product Collection block. Is there any reason you mentioned only query and not all attributes? I will wait for @xristos3490 response on this 🙂

Based on [this discussion](#45703 (comment)), I added a cleanup function support for handlePreviewState. `handlePreviewState` can return a function which will be called on cleanup in `useLayoutEffect` hook.
@xristos3490
Copy link
Member

xristos3490 commented Apr 1, 2024

Feedback looking good so far 👏

Before answering the params question, perhaps a silly question, but when will this handler run? Will it only run once, and if so, how can we retrigger the process if the query (or any other attribute changes?)

Examples that should be supported in core:

  • Moving the block in or out of a Single Product container
  • (Future) Setting a specific product context on the block level.
  • Others, like perhaps every time the query changes?

I believe we can handle cleanups, etc, on rerunning this function, but coming back to the params question: Will choosing params determine how often the cleanup and trigger happens? For example, running every time an attribute or the query changes?

cc @kmanijak

@github-actions github-actions bot added the plugin: woocommerce Issues related to the WooCommerce Core plugin. label Apr 2, 2024
- Consolidated `handlePreviewState` and `initialPreviewState` into a single `preview` prop in `register-product-collection.tsx` and `product-collection-content.tsx` to streamline prop passing and improve the component interface.
- Updated the `queryContextIncludes` in `constants.ts` to include 'previewState'
- Enhanced the `ProductCollection` PHP class to handle preview-specific queries more effectively, introducing a new method `get_preview_query_args` that adjusts query parameters based on the collection being previewed, thereby improving the relevance and accuracy of products displayed in preview mode.
- Renamed `HandlePreviewStateArgs` to `SetPreviewStateArgs` in `featured.tsx` to better reflect its purpose, which is now more focused on setting rather than handling states. The implementation details within `featured.tsx` have also been refined to include async operations and cleanup functions, demonstrating a more sophisticated approach to managing state.

Overall, these updates make the preview state logic more understandable and maintainable.
@imanish003
Copy link
Contributor Author

Hey Folks 👋

I've made some updates to the PR based on your feedback, and I'm excited to share a few examples of how the preview mode will function after the changes:

Example 1:
Here's how you can access attributes and location in the preview state, utilize async operations, and implement a cleanup function as a return value:

/**
 * Example:
 * - How to access attributes and location in the preview state?
 * - How to use async operations?
 * - How to use cleanup function as a return value?
 */
registerProductCollection(
	...,
	preview: {
		setPreviewState: ( {
			setState,
			attributes: currentAttributes,
			location,
		}: SetPreviewStateArgs ) => {
			// setPreviewState has access to the current attributes and location.
			console.log( 'setPreviewState' );
			console.log( currentAttributes, location );

			const timeoutID = setTimeout( () => {
				setState( {
					isPreview: false,
					previewMessage: '',
				} );
			}, 5000 );

			return () => clearTimeout( timeoutID );
		},
		initialState: {
			isPreview: true,
			previewMessage: 'This is in preview mode',
		},
	}
)

Example 2:
And here, we're just setting the initial state:

registerProductCollection(
	...,
	preview: {
		initialState: {
			isPreview: true,
			previewMessage: 'On sale Collection Preview',
		},
	},
)

Major Changes:

Show random products in Preview Mode

Before diving into how it's implemented, let's clarify what "random products" mean. Currently, it's fetching random products from existing items in the database. In the future, we might consider adding dummy products for cases where the database is empty. We could look into this in subsequent PRs. What are your thoughts?
Now, As you can see in this code:

// Is this a preview request?
// If yes, short-circuit the query and return the preview query.
$product_collection_query_context = $request->get_param( 'productCollectionQueryContext' );
$is_preview                       = $product_collection_query_context['previewState']['isPreview'] ?? false;
if ( 'true' === $is_preview ) {
	return $this->get_preview_query_args( $args, $request );
}

We check in PHP if current request is preview request. If it's a preview request, we can return custom WP_Query args. This way, different collection can have different random products. For example, On-sale collection should always return on-sale random products because on-sale products have "Sale" badge on it.
As you can see get_preview_query_args implementation, it allow us to have collection specific queries for random products. But we can always return a generic query if collection-specific query isn't needed. Later, We can introduce hook like rest_product_collection_preview_query for 3PDs.

Implemented Cleanup function

Based on #45703 (comment), I added cleanup function support in handlePreviewState. Now, handlePreviewState can return a function that will be called on cleanup in the useLayoutEffect hook. This cleanup function can be used to remove event listeners, clear timeouts, and handle other side effects.

When setPreviewState will be triggered?

  1. On initial load: When either Collection is added or editor page is refreshed, then setPreviewState will be triggered.
  2. When block is moved: IMO it's needed because location of block might change when block is moved. For example, if it's moved to Single Products block then location type will become:
{
  type: "product",
  sourceData: {
    productId: 2777
  }
};

Question: Should we trigger it whenever any attribute value changes, or specifically when the "query" attribute changes?

What isn't part of this PR?

Preview argument validation

In followup PR, we will add validation for preview argument passed in registerProductCollection function. For now, this is still a private API, but before making it public, we will need to add some validation, as incorrect data is possible.

Return dummy products instead of real products from DB

We could display dummy products when the database is empty in preview mode. However, I believe this would require further consideration, so I don't think it's something we want to implement for sure. I would love to hear your thoughts on this.

Required context

As mentioned by Karol in #45703 (review), collection should be able to specify requiredContext or requiredLocation. In case it's not satisfied we should not even call setPreviewState and set the preview ourselves.
For now, there is no way for collection to specify require context but when it will be implemented, we will need to handle this case, i.e. we will need to show preview label when required context isn't satisfied.

Feedback Needed

I'm eager to hear your thoughts on these updates. If there are no major concerns, I'll proceed to close this POC and prepare a new PR for final review. 🙂

@samueljseay
Copy link
Contributor

@imanish003 I really like all your changes and I think even the naming of everything is looking good and intuitive. Great stuff imo.

Copy link
Contributor

@kmanijak kmanijak left a comment

Choose a reason for hiding this comment

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

First of all, great work Manish! 🚀

Show random products in Preview Mode
Currently, it's fetching random products from existing items in the database. In the future, we might consider adding dummy products for cases where the database is empty. We could look into this in subsequent PRs. What are your thoughts?

I don't think adding dummy products brings much value so I wouldn't spend much time even considering it. Displaying some products from a store should be enough for now.

When setPreviewState will be triggered?
Question: Should we trigger it whenever any attribute value changes, or specifically when the "query" attribute changes?

Hmm... for now I'd stick to the current logic so invoking function on initial load and when the block is moved. If requested, we can change that logic later.


I'm eager to hear your thoughts on these updates. If there are no major concerns, I'll proceed to close this POC and prepare a new PR for final review. 🙂

I love what we have here and suggest progressing further. I left some comments for you to consider when finalizing/productionizing the solution! 🙌

Comment on lines +156 to +159
// Let's not render anything until default attributes are set.
if ( ! isInitialAttributesSet.current ) {
return null;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice one! I think we should adopt a similar mechanism for location in case it needs to invoke getEntityRecords in which two re-renders happen and because of that two calls.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do you mean changing this condition to the following?

if ( ! isInitialAttributesSet.current & !location) {

location in case it needs to invoke getEntityRecords

I don't think this is the case now. Do you mean in future we may call getEntityRecords in useGetLocation hook? Am I missing something? 🤷🏻‍♂️

This commit addresses an issue in the product-collection-content.tsx where the newPreviewState was not properly merged into the existing previewState attribute. Previously, the spread operator was incorrectly applied, leading to potential loss of existing state attributes. By changing the order of operations and correctly spreading the existing attributes before merging the newPreviewState, we ensure that all state attributes are preserved and updated correctly.
@imanish003
Copy link
Contributor Author

Thank you, everyone, for the fantastic feedback on this PR. I am closing this POC PR and will open a new one to move it to production 🙌🏻

@imanish003 imanish003 closed this Apr 8, 2024
imanish003 added a commit that referenced this pull request May 15, 2024
* POC: Preview Mode using HOC

* Add explanation as comments

POC: Implement preview mode for Product Collection block in editor
- Added extensive commentary to clarify the mechanism and usage of the `handlePreviewState` function
- Implemented an internal state within `ProductCollectionContent` to manage preview status and messages, serving as a foundational example of how preview mode can enrich block functionality.
- Showcased the application of `handlePreviewState` by incorporating it as a prop in `BlockEdit`, illustrating the potential for extending the block's capabilities for dynamic and interactive previews.

This POC demonstrates a flexible approach to managing preview states within the editor, paving the way for further development and integration based on feedback and use-case analysis.

* Refactor preview state handling and collection registration

This commit introduces a centralized approach for registering product collection variations and managing their preview states. It abstracts the registration logic into a dedicated function and enhances the flexibility of preview state handling across different collection types.

* Rename file

* Minor improvements

* Don't pass previewState to handlePreviewState

I don't see any good use of it in handlePreviewState. Also, We will be going to call handlePreviewState only once
therefore, it will always have the same value as the initial value of the previewState. If in future, we decide to run it
multiple times then we can pass the previewState as an argument to handlePreviewState.

* Add comment

* Use JS closure to inject handlePreviewState

This commit introduces a refined approach for injecting the `handlePreviewState` function into product collection blocks, utilizing JavaScript closures to streamline the process. This method replaces the previous global registry mechanism, offering a more direct and efficient way to manage preview states.

Advantages of This Approach:
- Utilizing JavaScript closures for injecting `handlePreviewState` simplifies the overall architecture by directly modifying block edit components without relying on an external registry. This method enhances code clarity and reduces the cognitive load for developers.
- The conditional application of `withHandlePreviewState` ensures that the preview state handling logic is only added to blocks that require it, optimizing performance and maintainability.

* Refactor preview state management into custom hook

This commit enhances the organization and readability of the product collection content component by abstracting the preview state management into a custom hook named `usePreviewState`. This change streamlines the component's structure and aligns with React best practices for managing state and side effects.

Key Changes:
- Introduced `usePreviewState`, a custom hook responsible for initializing and managing the preview state (`isPreview` and `previewMessage`) of the product collection block. This hook encapsulates the state logic and its side effects, including the conditional invocation of `handlePreviewState`.
- Modified `ProductCollectionContent` to utilize `usePreviewState` for handling its preview state. This adjustment makes the component cleaner and focuses it more on presentation and behavior rather than state management details.

* Replace useEffect with useLayoutEffect

* Add cleanup function in handlePreviewState function

Based on [this discussion](#45703 (comment)), I added a cleanup function support for handlePreviewState. `handlePreviewState` can return a function which will be called on cleanup in `useLayoutEffect` hook.

* Fetching random products in Preview mode

* Allow collection to set initial preview state

* Pass location & all attributes to handlePreviewState function

* Handling collection specific query for preview mode

- Consolidated `handlePreviewState` and `initialPreviewState` into a single `preview` prop in `register-product-collection.tsx` and `product-collection-content.tsx` to streamline prop passing and improve the component interface.
- Updated the `queryContextIncludes` in `constants.ts` to include 'previewState'
- Enhanced the `ProductCollection` PHP class to handle preview-specific queries more effectively, introducing a new method `get_preview_query_args` that adjusts query parameters based on the collection being previewed, thereby improving the relevance and accuracy of products displayed in preview mode.

* Always set initialPreviewState on load

* Refine preview state handling

- Renamed `HandlePreviewStateArgs` to `SetPreviewStateArgs` in `featured.tsx` to better reflect its purpose, which is now more focused on setting rather than handling states. The implementation details within `featured.tsx` have also been refined to include async operations and cleanup functions, demonstrating a more sophisticated approach to managing state.

Overall, these updates make the preview state logic more understandable and maintainable.

* Rename "initialState" to "initialPreviewState"

* Fix: Correct merging of newPreviewState into previewState attribute

This commit addresses an issue in the product-collection-content.tsx where the newPreviewState was not properly merged into the existing previewState attribute. Previously, the spread operator was incorrectly applied, leading to potential loss of existing state attributes. By changing the order of operations and correctly spreading the existing attributes before merging the newPreviewState, we ensure that all state attributes are preserved and updated correctly.

* Initial refactor POC code to productionize it

* Move `useSetPreviewState` to Utils

* Implement preview mode for Generic archive templates

Implemented a new useLayoutEffect in `utils.tsx` to dynamically set a preview message in the editor for product collection blocks located in generic archive templates (like Products by Category, Products by Tag, or Products by Attribute).

* Remove preview mode from Featured and On sale collection

* Remove preview query modfication for On Sale collection

* Add changefile(s) from automation for the following project(s): woocommerce-blocks, woocommerce

* Fix: hide/show preview label based on value of "inherit"

If user change the toggle "Sync with current query", then it should reflect for the preview label as well.
- If the toggle is on, then the preview label should be shown.
- If the toggle is off, then the preview label should be hidden.

* Minor improvements

* Add changefile(s) from automation for the following project(s): woocommerce-blocks, woocommerce

* Add changefile(s) from automation for the following project(s): woocommerce-blocks, woocommerce

* Refactor: Simplify SetPreviewState type definition in types.ts

This commit refines the SetPreviewState type definition by eliminating the previously used intermediate interface, SetPreviewStateArgs. The change streamlines the type definition directly within the SetPreviewState type, enhancing readability and reducing redundancy.

* Update import syntax for ElementType in register-product-collection.tsx

This commit updates the import statement for `ElementType` from `@wordpress/element` to use the more modern and concise `import type` syntax. This change does not affect functionality but aligns with TypeScript best practices for importing types, ensuring that type imports are distinguished from regular imports. This helps in clarity and in optimizing the build process by explicitly indicating that `ElementType` is used solely for type checking and not included in the JavaScript runtime.

* Refactor: Update TypeScript usage in Product Collection

This commit introduces several TypeScript refinements across product-collection components:

1. **DEFAULT_ATTRIBUTES** in `constants.ts` now uses `Pick` to explicitly define its shape, ensuring only relevant attributes are included and typed accurately.
2. **ProductCollectionAdvancedInspectorControls** and **ToolbarControls** in the `edit` subdirectory now use `Omit` to exclude the 'preview' property from props, clarifying the intended prop usage and improving type safety.

These changes collectively tighten the type definitions and improve the codebase's adherence to best practices in TypeScript.

* Refactor: Update dependencies of useSetPreviewState hook in utils.tsx

This change enhances the stability and predictability of the hook's behavior, ensuring it updates its internal state accurately when its dependencies change, thus aligning with best practices in React development.

* Refactor preview button CSS and conditional rendering

1. **CSS Refactoring:** Moved the positioning styles of the `.wc-block-product-collection__preview-button` from inline styles in the JSX to the `editor.scss` file. This separation of concerns improves maintainability and readability, aligning the styling responsibilities solely within the CSS file.
2. **Conditional Rendering Logic:** Updated the rendering condition for the preview button. Now, it not only checks if `isPreview` is true but also if the block is currently selected (`props.isSelected`). This prevents the preview button from appearing when the block is not actively selected, reducing visual clutter and enhancing the user experience in the editor.

* Enhance: Update preview button visibility logic in ProductCollectionContent

This commit enhances the visibility logic of the preview button in the `ProductCollectionContent` component:

1. **Added `isSelectedOrInnerBlockSelected`:** Introduced a new `useSelect` hook to determine if the current block or any of its inner blocks are selected. This ensures that the preview button is visible when either the product collection block or any of its inner blocks are selected.
2. **Updated Conditional Rendering:** Adjusted the conditional rendering of the preview button to use the new `isSelectedOrInnerBlockSelected` value, providing a more intuitive user experience by ensuring the preview button remains visible when any relevant block is selected.

* use __private prefix with attribute name

* Add E2E tests for Preview Mode

1. **Template-Specific Tests:** Each template (tag, category, attribute) undergoes a test to ensure the preview button behaves as expected when replacing products with product collections in these contexts.
2. **Visibility Checks:** The tests verify that the preview button is visible when the block or its inner blocks are selected and hidden when the block is not selected. This helps confirm the correct implementation of the preview button visibility logic across different use cases.
3. **Interaction with Inner Blocks:** Additional checks are included to ensure the preview button's visibility toggles appropriately when interacting with inner blocks, reinforcing the dynamic nature of block selection and its effect on UI elements within the editor.

* Add setPreviewState to dependencies

* Add data-test-id to Preview button and update e2e locator

Modifications:
- Added `data-test-id="product-collection-preview-button"` to the Preview button in `product-collection-content.tsx`.
- Updated the corresponding e2e test locator in `product-collection.block_theme.side_effects.spec.ts` to use the new `data-test-id` instead of the class name.

By using `data-test-id`, we ensure that the e2e tests are not affected by changes in the styling or restructuring of the DOM that might alter CSS classes but do not affect functionality.

* Enhance: Localize preview message in useSetPreviewState hook

* Don't show shadow & outline on focus

* Make preview button font same as Admin

* Fix SCSS lint errors

* Add missing await keyword

---------

Co-authored-by: github-actions <github-actions@github.com>
@imanish003 imanish003 deleted the try/product-collection-is-preview-POC branch May 17, 2024 06:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
plugin: woocommerce Issues related to the WooCommerce Core plugin.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Product Collection: Implement POC for Preview Mode
4 participants