-
Notifications
You must be signed in to change notification settings - Fork 10.7k
Disable product inventory toggle when inventory management is disabled #35059
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
Conversation
Test Results SummaryCommit SHA: 15e32b6
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. |
| <> | ||
| <ToggleControl | ||
| label={ __( | ||
| <BaseControl |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We are using BaseControl and a FormToggle here instead of ToggleControl for 2 reasons:
- Hovering over
ToggleControldoes not display a tooltip due to the disabled property. We can skirt this by adding a div wrapper but this throws off our product page styling slightly by wrapping some of the Gutenberg classes. - Using
ToggleControldoes not allow us to wrap just the toggle which appears to be the case from the designs cc @jarekmorawski
I'm not overly opinionated on this, so if want to revert to ToggleControl and create another overlay to circumvent the GB disabled issues, I'll be happy to try that out.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the ping, Josh. Is the problem just with the element to wrap or the whole component? If the former, it's okay to treat the whole element (toggle + label) as disabled and show the tooltip on hover.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jarekmorawski there are two issues, but the only one that is potentially a concern on the design side is the tooltip not being centered over the toggle itself, but centered over the whole element (toggle + label). The way it's setup right now, the tooltip displays on hovering the toggle only and is centered over that toggle.
If centering the tooltip on over the whole element and displaying it also on label hover is acceptable, this opens up the alternative to the other solution (dev note: not necessarily the best solution based on the above criteria).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If centering the tooltip on over the whole element and displaying it also on label hover is acceptable, this opens up the alternative to the other solution
Given that the user cannot interact with the toggle directly on the page, for accessibility reasons, I'd vote for showing the tooltip for the whole element. What do you think would be the best solution?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If centering the tooltip over the entire element is not an issue in terms of design, that's an option.
One alternative might be to add our version of the tooltip, a help icon that's clickable (used in other areas of the new product experience) if we want more control over what's rendered.
I think both are fine solutions; let me know if you have a preference towards either.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If centering the tooltip over the entire element is not an issue in terms of design, that's an option.
Let's go with this option. It's simple enough and we're not building unnecessary dependencies or forking components. 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It does look odd. 🤔 Yeah, let's have the tooltip center right over the checkbox and label.
louwie17
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@joshuatf this PR is a bit difficult to review as it appears to be behind on a rebase and a bunch of other changes are showing up in the Diff.
Are you able to rebase this?
|
Hey @louwie17 sorry about the mess earlier. This one has been rebased and is ready for a look when you have time. |
louwie17
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This tested well and code looks good, thanks for fixing the popover location as well, LGTM 🚀
|
Hi @joshuatf, thanks for merging this pull request. Please take a look at these follow-up tasks you may need to perform:
|

All Submissions:
Changes proposed in this Pull Request:
Disables the toggle when site wide stock management is disabled. Also adds a tooltip to the toggle when disabled.
Closes #35046 .
How to test the changes in this Pull Request:
wp-admin/admin.php?page=wc-settings&tab=products§ion=inventoryProducts -> Add new (MVP)Other information:
pnpm --filter=<project> run changelog add?FOR PR REVIEWER ONLY: