woocommerce icon indicating copy to clipboard operation
woocommerce copied to clipboard

[CYS - Full Composability]: Add tooltip hovered pattern

Open gigitux opened this issue 1 year ago • 1 comments

Submission Review Guidelines:

Changes proposed in this Pull Request:

Closes # .

How to test the changes in this Pull Request:

Using the WooCommerce Testing Instructions Guide, include your detailed testing instructions:

Changelog entry

  • [ ] Automatically create a changelog entry from the details below.

Significance

  • [ ] Patch
  • [ ] Minor
  • [ ] Major

Type

  • [ ] Fix - Fixes an existing bug
  • [ ] Add - Adds functionality
  • [ ] Update - Update existing functionality
  • [ ] Dev - Development related task
  • [ ] Tweak - A minor adjustment to the codebase
  • [ ] Performance - Address performance issues
  • [ ] Enhancement - Improvement to existing functionality

Message

Comment

gigitux avatar May 17 '24 13:05 gigitux

Test using WordPress Playground

The changes in this pull request can be previewed and tested using a WordPress Playground instance. WordPress Playground is an experimental project that creates a full WordPress instance entirely within the browser.

Test this pull request with WordPress Playground.

Note that this URL is valid for 30 days from when this comment was last updated. You can update it by closing/reopening the PR or pushing a new commit.

github-actions[bot] avatar May 17 '24 13:05 github-actions[bot]

Size Change: 0 B

Total Size: 2.47 MB

compressed-size-action

github-actions[bot] avatar May 31 '24 10:05 github-actions[bot]

Hi @albarin, @nefeline,

Apart from reviewing the code changes, please make sure to review the testing instructions as well.

You can follow this guide to find out what good testing instructions should look like: https://github.com/woocommerce/woocommerce/wiki/Writing-high-quality-testing-instructions

github-actions[bot] avatar Jun 03 '24 10:06 github-actions[bot]

I noticed that enabling the mousemove event , the console throws multiple errors:

This is caused by the useInBetweenInserter hook. This hook is necessary to show the inserter. In this case, we don't need the inserter feature, so it isn't a big deal. However, it could be noisy, so if you agree with me, I will open a dedicated issue when the PR is merged.

Also, I think that the quickest fix is to change this check to if ( openRef?.current ) {

gigitux avatar Jun 03 '24 14:06 gigitux

Not a blocker, but a thought: whenever hovering over the sidebar and going back to the main frame, I see the tooltip is still displayed even without any new click on the pattern: should we change this to, if the pointer is moved away from the frame and then back, the tooltip is displayed again only on click?

Thanks for catching this, @nefeline!

I agree with you. Given that the PR is already very big, I'm going to create a dedicated issue to fix this!

gigitux avatar Jun 05 '24 12:06 gigitux