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.

Sign up for GitHub

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

Jump to bottom

Hide Variations section when it is empty #36202

Merged
merged 4 commits into from Jan 12, 2023
Merged

Hide Variations section when it is empty #36202

merged 4 commits into from Jan 12, 2023

Conversation

octaedro
Copy link
Contributor

@octaedro octaedro commented Dec 27, 2022 •
edited

All Submissions:

Changes proposed in this Pull Request:

Hide the Variations section when there aren't any variations to show.

Closes #36181.

  • This PR is a very minor change/addition and does not require testing instructions (if checked you can ignore/remove the next section).

How to test the changes in this Pull Request:

  1. Navigate to the new product management experience (Products -> Add new (MVP))
  2. Click the "Options" tab
  3. The Variations section should be hidden.
  4. Go to a product with variations wp-admin/admin.php?page=wc-admin&path=/product/{product_id}.
  5. Click the "Options" tab and verify that the Variations section is being shown correctly.

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 created a changelog file for each project being changed, ie pnpm --filter=<project> changelog add?

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.

@github-actions github-actions bot added focus: react admin plugin: woocommerce Issues related to the WooCommerce Core plugin. labels Dec 27, 2022
@github-actions
Copy link
Contributor

github-actions bot commented Dec 27, 2022 •
edited

Test Results Summary

Commit SHA: e9d7f4d

Test 🧪Passed ✅Failed 🚨Broken 🚧Skipped ⏭️Unknown ❔Total 📊Duration ⏱️
API Tests25900202610m 52s
E2E Tests189006019514m 34s
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.

@octaedro octaedro requested a review from a team December 27, 2022 19:27
@octaedro octaedro self-assigned this Dec 27, 2022
@octaedro octaedro marked this pull request as ready for review December 27, 2022 19:28
joshuatf
joshuatf reviewed Dec 28, 2022
View reviewed changes
Copy link
Contributor

@joshuatf joshuatf 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 creating this issue and tackling it, Fernando!

I'm still seeing the variations section shown with this PR checked out. I think we might need check for the existence of variations from the data store inside the variations section.
Screen Shot 2022-12-27 at 4 58 12 PM

@octaedro octaedro mentioned this pull request Dec 28, 2022
Merged
8 tasks
@octaedro octaedro added status: blocked The issue is blocked from progressing, waiting for another piece of work to be done. and removed status: blocked The issue is blocked from progressing, waiting for another piece of work to be done. labels Dec 29, 2022
@octaedro octaedro force-pushed the fix/36181_hide_variations_section_when_empty branch from bdedf7b to eaaf86b Compare January 5, 2023 15:37
@octaedro octaedro requested a review from joshuatf January 5, 2023 15:37
@octaedro
Copy link
Contributor Author

octaedro commented Jan 5, 2023

Thank you @joshuatf for reviewing this PR.
I addressed the suggestion you made in your comment. Could you take another look at this PR?

joshuatf
joshuatf reviewed Jan 6, 2023
View reviewed changes
Copy link
Contributor

@joshuatf joshuatf 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 the update! Looks good for each case of options existing or not, but ran into a little bit of trouble when changing the options.

Could you check if that's happening on your end? Left a couple possible solutions in the comments.

plugins/woocommerce-admin/client/products/sections/product-variations-section.tsx Outdated
const { values } = useFormContext< Product >();
const productId = values.id;

const { totalCount } = useSelect(
Copy link
Contributor

Choose a reason for hiding this comment

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

This is working well for me on the default state, but when options are changed by deleting them if they already existed or by adding them on a new product, the variations section does not change.

We might need cache invalidation for this store when options are changed or maybe we should just use the same getProductAttributes request that is used in options and check that there is at least 1 item.

joshuatf
joshuatf reviewed Jan 10, 2023
View reviewed changes
Copy link
Contributor

@joshuatf joshuatf 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 the updated logic!

I left one case where things are broken on a new product page, but left a comment on how we can fix it. Let me know if that works and we can approve if so.

plugins/woocommerce-admin/client/products/sections/product-variations-section.tsx Outdated
[ productId, options ]
);

if ( options.length === 0 || totalCount === 0 || isNaN( totalCount ) ) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we can remove all of this totalCount logic now and just rely on the options.length.

On a new product page when adding options, this totalCount will not get updated and the variations will remain hidden.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you @joshuatf for your review.

This is interesting; when I place a breakpoint here the totalCountconst is loaded correctly after adding a new option. I guess that there is something related to the variations creation time.

As you mentioned, it seems like we can remove the totalCount. I removed that logic in the commit e9d7f4d

joshuatf
joshuatf approved these changes Jan 12, 2023
View reviewed changes
Copy link
Contributor

@joshuatf joshuatf left a comment

Choose a reason for hiding this comment

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

Testing well on my end and LGTM! 🚢

@octaedro octaedro merged commit 78002ea into trunk Jan 12, 2023
@octaedro octaedro deleted the fix/36181_hide_variations_section_when_empty branch January 12, 2023 19:32
@github-actions github-actions bot added this to the 7.4.0 milestone Jan 12, 2023
louwie17 pushed a commit that referenced this pull request Jan 13, 2023
@octaedro @louwie17
Hide Variations section when it is empty (#36202)
9a0efa9 
* Add changelog

* Hide Variations section when it is empty

* Fix hiding variations

* Remove `totalCount const

Co-authored-by: Fernando Marichal <contacto@fernandomarichal.com>
@github-actions github-actions bot mentioned this pull request Feb 15, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@joshuatf joshuatf joshuatf approved these changes

Assignees

@octaedro octaedro

Labels
plugin: woocommerce Issues related to the WooCommerce Core plugin.
Projects
None yet
Milestone
7.4.0
Development

Successfully merging this pull request may close these issues.

[Enhancement]: Variations section is visible even when there are no variations to show
2 participants
@octaedro @joshuatf