Skip to content

Conversation

@joshuatf
Copy link
Contributor

@joshuatf joshuatf commented Oct 12, 2022

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.

Screen Shot 2022-10-12 at 12 53 35 PM

Closes #35046 .

How to test the changes in this Pull Request:

  1. Disable stock management at wp-admin/admin.php?page=wc-settings&tab=products&section=inventory
  2. Navigate to Products -> Add new (MVP)
  3. Check that the toggle to enable inventory management is disabled
  4. Hover the toggle to see the tooltip
  5. Enable site-wide stock management again
  6. Make sure the toggle once again works
  7. Check that hovering the toggle does not display the tooltip

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 --filter=<project> run 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.

@joshuatf joshuatf self-assigned this Oct 12, 2022
@github-actions github-actions bot added focus: react admin type: community contribution release: highlight Issues that have a high user impact and need to be discussed/paid attention to. labels Oct 12, 2022
@github-actions github-actions bot added the package: @woocommerce/components issues related to @woocommerce/components label Oct 12, 2022
@joshuatf joshuatf marked this pull request as ready for review October 12, 2022 20:12
@github-actions
Copy link
Contributor

github-actions bot commented Oct 12, 2022

Test Results Summary

Commit SHA: 15e32b6

Test 🧪Passed ✅Failed 🚨Broken 🚧Skipped ⏭️Unknown ❔Total 📊Duration ⏱️
API Tests20000202020m 44s
E2E Tests186006019218m 15s

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.

@joshuatf joshuatf requested a review from a team October 12, 2022 20:12
<>
<ToggleControl
label={ __(
<BaseControl
Copy link
Contributor Author

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:

  1. Hovering over ToggleControl does 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.
  2. Using ToggleControl does 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.

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.

Copy link
Contributor Author

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).

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?

Copy link
Contributor Author

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.

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. 👍

Copy link
Contributor

Choose a reason for hiding this comment

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

This does cause the tooltip to look a bit odd as the checkbox and label take up the full width of the field input, making the tooltip appear further on the right:
Screen Shot 2022-10-20 at 10 52 54 AM

Would it make sense for the checkbox and label not to be full width, so the tooltip centers correctly over it?

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.

@github-actions github-actions bot removed the release: highlight Issues that have a high user impact and need to be discussed/paid attention to. label Oct 17, 2022
@github-actions github-actions bot added package: @woocommerce/data issues related to @woocommerce/data plugin: woocommerce Issues related to the WooCommerce Core plugin. labels Oct 19, 2022
Copy link
Contributor

@louwie17 louwie17 left a 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?

@joshuatf
Copy link
Contributor Author

Thanks for looking, @louwie17. I'm kind of in rebase hell until #35047 gets merged. I'll ping you as soon as that gets merged and I can fix this one up.

Base automatically changed from add/34388 to trunk October 20, 2022 21:14
@github-actions github-actions bot removed plugin: woocommerce Issues related to the WooCommerce Core plugin. package: @woocommerce/data issues related to @woocommerce/data labels Oct 20, 2022
@joshuatf
Copy link
Contributor Author

Hey @louwie17 sorry about the mess earlier. This one has been rebased and is ready for a look when you have time.

@github-actions github-actions bot added the plugin: woocommerce Issues related to the WooCommerce Core plugin. label Oct 20, 2022
Copy link
Contributor

@louwie17 louwie17 left a 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 🚀

@joshuatf joshuatf merged commit 2aa4ce0 into trunk Oct 24, 2022
@joshuatf joshuatf deleted the add/35046 branch October 24, 2022 16:08
@github-actions github-actions bot added this to the 7.2.0 milestone Oct 24, 2022
@github-actions
Copy link
Contributor

Hi @joshuatf, 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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

package: @woocommerce/components issues related to @woocommerce/components plugin: woocommerce Issues related to the WooCommerce Core plugin. type: community contribution

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Inventory] Disable inventory control toggle when inventory management is disabled

4 participants