Convert all products edit to TypeScript #9782

Sidsector9 · 2023-06-11T20:14:54Z

Fixes woocommerce/woocommerce#42363

Testing

Automated Tests

Changes in this PR are covered by Automated Tests.
- Unit tests
- E2E tests
Do not include in the Testing Notes

WooCommerce Visibility

WooCommerce Core
Feature plugin
Experimental

…s into upkeep/9524

nielslange

Thanks for your contribution, @Sidsector9. I requested a review from the corresponding team. While quickly looking over the PR, I noticed one section, that should be changed. I left a comment in the corresponding section.

nielslange · 2023-06-14T13:15:49Z

assets/js/blocks/products/all-products/edit.tsx

-		debouncedSpeak: PropTypes.func.isRequired,
-	};
-
+class Editor extends Component< EditorProps, EditorState > {


While briefly looking at your changes, I noticed that this file is still using a class component. Now that we're touching this file, we should convert it to a functional component. Could you take care of that?

@nielslange I've moved from class to functional component.

assets/js/blocks/products/all-products/edit.tsx

…s into upkeep/9524

kmanijak

Thanks for contributing! I left a couple of comments related to the code as well as to the behavior of All Products block.

I would also suggest adding some testing steps to the PR description to showcase how did you check that this block preserves the behavior from before the refactor. It also makes it easier for us to confirm it works the same on our end, thanks!

kmanijak · 2023-06-20T06:59:32Z

assets/js/blocks/products/all-products/edit.tsx

+			template: [ string, object? ][];
+			templateLock: boolean;
+			allowedBlocks: Array< string >;
+			renderAppender?: undefined | boolean;


Considering the property is optional, it can be implicitly undefined hence you can just define it as optional boolean:

Suggested change

renderAppender?: undefined | boolean;

renderAppender?: boolean;

kmanijak · 2023-06-20T07:40:58Z

assets/js/blocks/products/all-products/edit.tsx

-	blockMap = getBlockMap( 'woocommerce/all-products' );
+export default function Edit( props: Props ): JSX.Element {
+	const [ isEditing, setIsEditing ] = useState< boolean >( false );
+	const [ , setInnerBlocks ] = useState< BlockInstance[] | boolean >( false );


To preserve the logic, I believe the initial state should be an empty array as per previous implementation:

state = { isEditing: false, innerBlocks: [], };

Also, could you shed more light on why there's just a setInnerBlocks function in use, but no innerBlocks value like:

Suggested change

const [ , setInnerBlocks ] = useState< BlockInstance[] | boolean >( false );

const [ innerBlocks, setInnerBlocks ] = useState< BlockInstance[] | boolean >( false );

kmanijak · 2023-06-20T07:42:56Z

assets/js/blocks/products/all-products/edit.tsx


-	getTitle = () => {
+	const { innerBlocks } = useSelect( ( select ) => {


I see the component logic is based on the value returned from useSelect, while it should be based on the value from useState. I think it's related to the question from the previous comment about why the innerBlocks from useState is not in use.

kmanijak · 2023-06-20T07:53:30Z

assets/js/blocks/products/all-products/edit.tsx

-			const { innerBlocks } = this.state;
-			replaceInnerBlocks( block.clientId, innerBlocks, false );
-			this.togglePreview();
+			replaceInnerBlocks( clientId, innerBlocks, false );


When you use Cancel button on the UI and go back to edit mode, the previous state is preserved. It may be related to using innerBlocks from useSelect rather than useState.

Steps to reproduce:

Go to Edit mode of All Products

Add some additional Products Elements

Click "Cancel"

In the preview mode there's no newly added elements

Go to Edit mode again

All the added elements are there while the edit mode should reflect the preview state

kmanijak · 2023-06-20T07:57:36Z

assets/js/blocks/products/all-products/save.js

What do you think about renaming the file to save.jsx considering it returns JSX component?

kmanijak · 2023-06-20T08:06:32Z

assets/js/blocks/products/all-products/edit.tsx

+	}, [] );
+
+	const { replaceInnerBlocks } = useDispatch( 'core/block-editor' );
+	const debouncedSpeak = useDebounce( speak );


Is there a reason we cannot use the debouncedSpeak provided through the props as it was before, but rather create a new one?

@kmanijak Let me know if I've understood it incorrectly. Previously, debouncedSpeak was the props added by the withSpokenMessages HOC and I've replaced that with the useDebounce hook. Is that not correct?

Previously, debouncedSpeak was the props added by the withSpokenMessages HOC and I've replaced that with the useDebounce hook. Is that not correct?

Oh, I didn't spot that. But I think we could stick to keep using withSpokenMessages which is used across multiple blocks. Is there a reason to replace it with a custom implementation?

…s into upkeep/9524

kmanijak

I tried to test the All Products block and after modifying the content All Products turns into some unexpected state:

allprod-hidden-content.mov

Also, as I mentioned in the previous iteration:

I would also suggest adding some testing steps to the PR description to showcase how did you check that this block preserves the behavior from before the refactor. It also makes it easier for us to confirm it works the same on our end, thanks!

Even though this PR doesn't add a new feature it helps us and other colleagues to make sure the changes don't introduce any regressions, so I'd appreciate if you add some testing steps to the PR description. Here are some examples of PR introducing a similar change (conversion to TypeScript) with the testing steps included:

Please let me know if you have some questions 🙌

kmanijak · 2023-06-29T14:16:12Z

assets/js/blocks/products/all-products/edit.tsx

+	}, [] );
+
+	const { replaceInnerBlocks } = useDispatch( 'core/block-editor' );
+	const debouncedSpeak = useDebounce( speak );


Previously, debouncedSpeak was the props added by the withSpokenMessages HOC and I've replaced that with the useDebounce hook. Is that not correct?

Oh, I didn't spot that. But I think we could stick to keep using withSpokenMessages which is used across multiple blocks. Is there a reason to replace it with a custom implementation?

kmanijak · 2023-06-29T14:31:58Z

assets/js/blocks/products/all-products/save.js

What do you think about renaming this file to save.jsx as it returns JSX content?

kmanijak · 2023-12-07T11:05:37Z

Hi @Sidsector9!

It’s been a while since we heard from you. I’d like you to know that this weekend (8-9 December) WooCommerce Blocks repo will be migrated to WooCommerce monorepo. However, open PRs won’t be migrated.

That means open PRs have to be merged by the end of this week or otherwise, you’ll have to open/recreate a new PR in a monorepo post-migration if you’re interested in finalizing this contribution.

Thanks!

migrate to typescript

0696a64

github-actions bot added the type: community contribution label Jun 11, 2023

Sidsector9 added 3 commits June 12, 2023 23:06

fix typescript errors

ed48fc3

Merge branch 'trunk' into upkeep/9524

0c99974

Merge branch 'upkeep/9524' of github.com:Sidsector9/woocommerce-block…

4b48aca

…s into upkeep/9524

Sidsector9 changed the title ~~[WIP] upkeep/9524: migrate to typescript~~ upkeep/9524: migrate to typescript Jun 12, 2023

Merge branch 'trunk' into upkeep/9524

40c5340

nielslange added type: enhancement The issue is a request for an enhancement. type: refactor The issue/PR is related to refactoring. block: all products Issues related to the all products block. labels Jun 14, 2023

nielslange requested review from a team and danieldudzic and removed request for a team June 14, 2023 13:12

nielslange suggested changes Jun 14, 2023

View reviewed changes

kmanijak reviewed Jun 14, 2023

View reviewed changes

assets/js/blocks/products/all-products/edit.tsx Outdated Show resolved Hide resolved

Sidsector9 added 3 commits June 16, 2023 19:43

Merge branch 'trunk' into upkeep/9524

7da0883

Merge branch 'upkeep/9524' of github.com:Sidsector9/woocommerce-block…

dca174b

…s into upkeep/9524

move from class to functional component

742ec90

Sidsector9 force-pushed the upkeep/9524 branch from e81ad0c to 742ec90 Compare June 16, 2023 19:17

Merge branch 'trunk' into upkeep/9524

8f30bd0

Sidsector9 requested review from nielslange and kmanijak June 16, 2023 19:18

Merge branch 'trunk' into upkeep/9524

df9d3f9

nielslange removed their request for review June 19, 2023 09:06

Merge branch 'trunk' into upkeep/9524

c1aae08

kmanijak suggested changes Jun 20, 2023

View reviewed changes

kmanijak reviewed Jun 20, 2023

View reviewed changes

Sidsector9 added 2 commits June 25, 2023 20:43

PR review changes

2ecc032

Merge branch 'upkeep/9524' of github.com:Sidsector9/woocommerce-block…

6261b4e

…s into upkeep/9524

Sidsector9 requested a review from kmanijak June 28, 2023 08:23

kmanijak suggested changes Jun 29, 2023

View reviewed changes

samueljseay changed the title ~~upkeep/9524: migrate to typescript~~ Convert all products edit to TypeScript Nov 14, 2023

samueljseay added the status: stale Stale issues and PRs have had no updates for 60 days. label Nov 14, 2023

kmanijak added the team: Kirigami & Origami label Dec 7, 2023

kmanijak removed the type: enhancement The issue is a request for an enhancement. label Dec 7, 2023

danieldudzic removed their request for review December 26, 2023 22:16

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Convert all products edit to TypeScript #9782

Convert all products edit to TypeScript #9782

Sidsector9 commented Jun 11, 2023 •

edited

Loading

nielslange left a comment

nielslange Jun 14, 2023

Sidsector9 Jun 16, 2023

kmanijak left a comment

kmanijak Jun 20, 2023

kmanijak Jun 20, 2023

kmanijak Jun 20, 2023

kmanijak Jun 20, 2023

kmanijak Jun 20, 2023

kmanijak Jun 20, 2023

Sidsector9 Jun 25, 2023

kmanijak Jun 29, 2023

kmanijak left a comment

kmanijak Jun 29, 2023

kmanijak Jun 29, 2023

kmanijak commented Dec 7, 2023

	renderAppender?: undefined \| boolean;
	renderAppender?: boolean;

	const [ , setInnerBlocks ] = useState< BlockInstance[] \| boolean >( false );
	const [ innerBlocks, setInnerBlocks ] = useState< BlockInstance[] \| boolean >( false );


		getTitle = () => {
		const { innerBlocks } = useSelect( ( select ) => {

Convert all products edit to TypeScript #9782

Are you sure you want to change the base?

Convert all products edit to TypeScript #9782

Conversation

Sidsector9 commented Jun 11, 2023 • edited Loading

Testing

Automated Tests

WooCommerce Visibility

nielslange left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kmanijak left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kmanijak left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kmanijak commented Dec 7, 2023

Sidsector9 commented Jun 11, 2023 •

edited

Loading