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

Disable action buttons when product form is invalid #34658

Merged
merged 6 commits into from
Sep 22, 2022

Conversation

mdperez86
Copy link
Contributor

@mdperez86 mdperez86 commented Sep 13, 2022

All Submissions:

Changes proposed in this Pull Request:

Closes 38-gh-woocommerce/mothra-private.

How to test the changes in this Pull Request:

  1. Visit the Add product (MVP) page
  2. All action buttons should be disabled

image

3. If at least one field is invalid all action buttons should become disabled

image

4. When form fields are valid, action buttons disable behavior should remain in accordance with other requirements

image

5. When editing a product if the form is invalid then Move to trash action button should be enabled

image

6. When adding a new product `Move to trash` action button should not be shown

image

7. `Move to trash` is only visible when editing the product

image

8. When editing a product if the name field has any error then the product link is not shown but the error message does. See #issuecomment-1248104111 down below.

image

9. Publish options menu should be disabled if all menu items are disabled

image

Other information:

  • Have you added an explanation of what your changes do and why you'd like us to include them?
  • Have you written new tests for your changes, as applicable?
  • Have you successfully run tests with your changes locally?
  • Have you created a changelog file for each project being changed, ie pnpm changelog add --filter=<project>?

FOR PR REVIEWER ONLY:

  • I have reviewed that everything is sanitized/escaped appropriately for any SQL or XSS injection possibilities. I made sure Linting is not ignored or disabled.

@mdperez86 mdperez86 requested review from octaedro, louwie17 and a team September 13, 2022 18:26
@github-actions github-actions bot added focus: react admin plugin: woocommerce Issues related to the WooCommerce Core plugin. labels Sep 13, 2022
@mdperez86 mdperez86 added focus: new product ux revamped product management experience and removed plugin: woocommerce Issues related to the WooCommerce Core plugin. focus: react admin labels Sep 13, 2022
@github-actions
Copy link
Contributor

github-actions bot commented Sep 13, 2022

Test Results Summary

Commit SHA: 6badcf9

Test 🧪Passed ✅Failed 🚨Broken 🚧Skipped ⏭️Unknown ❔Total 📊Duration ⏱️
API Tests11800201201m 0s
E2E Tests185002018714m 36s
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.

@@ -217,7 +226,7 @@ export const ProductFormActions: React.FC = () => {
<MenuItem
onClick={ onTrash }
isDestructive
disabled={ ! values.id }
disabled={ ! isValidForm || ! values.id }
Copy link
Contributor

@octaedro octaedro Sep 14, 2022

Choose a reason for hiding this comment

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

I think that this action should be allowed even if the form is not valid. Leaving this line how it was before should be enough.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Your are right @octaedro, this change is done now.
But my suggestion is to hide the Move to trash action button from the creation page because it does not make sense to show an action to an user that will never be enabled in that page. That action is only needed in the edition page. Does that make sense for you @pmcpinto and @jarekmorawski?

Choose a reason for hiding this comment

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

It does. 👍 We can hide it for now and bring it back if we hear from the users they need it (which I don't think will happen). We can only show the button once the product is saved as draft or published.

Copy link
Contributor

@octaedro octaedro left a comment

Choose a reason for hiding this comment

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

Good job @mdperez86! I just left a small comment. Other than that the code looks good and tests well here.

@mdperez86 mdperez86 force-pushed the add/38-disable-action-buttons branch from 1f9dc4b to d873e8a Compare September 14, 2022 14:58
@github-actions github-actions bot added focus: react admin plugin: woocommerce Issues related to the WooCommerce Core plugin. labels Sep 14, 2022
octaedro
octaedro previously approved these changes Sep 14, 2022
Copy link
Contributor

@octaedro octaedro left a comment

Choose a reason for hiding this comment

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

Thank you @mdperez86 for adding the change! LGTM 🚀

@jarekmorawski
Copy link

  1. When editing a product if the form is invalid then Move to trash action button should be enabled

Can we hide the slug when the validation message is shown? It currently doesn't look very good when both of them are in the form.

@jarekmorawski
Copy link

Looks good, @mdperez86 💪 One more small thing: can we disable the arrow in the split button when all options inside are unavailable? We'd re-enable it if at least 1 option there is active.

@mattsherman mattsherman self-requested a review September 21, 2022 20:16
@mdperez86 mdperez86 force-pushed the add/38-disable-action-buttons branch from baae459 to da11ddf Compare September 21, 2022 20:39
@mattsherman
Copy link
Contributor

This is not specific to this PR... it appears to be the default of how Gutenberg Button is styled... I do not care for how the button still appears interactive even when disabled. It reacts when you click on it. This could be confusing.

Disabled buttons still appear interactive

/cc @jarekmorawski

Copy link
Contributor

@mattsherman mattsherman left a comment

Choose a reason for hiding this comment

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

Nice job! Especially with the tests.

Left a comment asking for the publish menu items test to be tweaked so that it doesn't depend on menu item order.

Copy link
Contributor

@mattsherman mattsherman 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 improving the publish options menu items test.

Looks great!

@mdperez86 mdperez86 merged commit 7b55c33 into trunk Sep 22, 2022
@mdperez86 mdperez86 deleted the add/38-disable-action-buttons branch September 22, 2022 20:47
@github-actions github-actions bot added this to the 7.1.0 milestone Sep 22, 2022
@github-actions
Copy link
Contributor

Hi @mdperez86, thanks for merging this pull request. Please take a look at these follow-up tasks you may need to perform:

  • Add the release: add testing instructions label

@jarekmorawski
Copy link

This is not specific to this PR... it appears to be the default of how Gutenberg Button is styled... I do not care for how the button still appears interactive even when disabled. It reacts when you click on it. This could be confusing.

Ideally, the button would not react to clicking. We could also change the cursor to not-allowed to better communicate that. As for the styling, I'm afraid that's how the component works.

Surprisingly, the WP Figma library doesn't even have a disabled state. If this doesn't sit with us, I think we've used the secondary disabled state for primary buttons in the past.

image

Konamiman pushed a commit that referenced this pull request Sep 23, 2022
* Disable action buttons when product form is invalid

* Move to trash action should be enabled when editing a product even when the form is invalid

* Hide Move to trash action button in the Add new product page

* Hide product link when the name field has any error

* Disable Publish options menu button when all menu items are disabled

* Test menu items by text instead of index
@AnnaMag
Copy link
Contributor

AnnaMag commented Sep 23, 2022

@jarekmorawski if there are more changes required, could we open a separate issue? This is to make sure no information is lost (due to e.g. PRs being closed).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
focus: new product ux revamped product management experience plugin: woocommerce Issues related to the WooCommerce Core plugin.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants