Skip to content

Image Gallery: Update toolbar position and tooltips #35534

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
Merged
mattsherman merged 4 commits into from
Nov 14, 2022
Merged

Image Gallery: Update toolbar position and tooltips #35534

mattsherman merged 4 commits into from
Nov 14, 2022

Conversation

@mattsherman
Copy link
Contributor

@mattsherman mattsherman commented Nov 9, 2022

All Submissions:

Changes proposed in this Pull Request:

The position of the toolbar has been adjusted to provide an 8px margin between the toolbar and the image. The z-index of the toolbar has also been updated to prevent the tooltips from appears behind images.

The toolbar tooltips have also been updated:

  • The drag handle tooltip says Drag to reorder
  • The left arrow icon tooltip says Move previous
  • The right arrow icon tooltip says Move next
  • The cover icon tooltip says Set as cover
  • The trash icon tooltip says Remove

Closes #35182
Closes #35184

Before

Screenshot 2022-11-09 at 22 37 42

  • Note the lack of margin between the image and the toolbar
  • Note the lack of the "Remove" tooltip... it is hiding behind the next image

After

Screenshot 2022-11-09 at 22 16 50

  • Note the margin between the image and the toolbar
  • Note the "Remove" tooltip appears properly
  • 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. Go to Products > Add new (MVP)
  2. Add a few images to the image gallery
  3. Verify that the toolbar appears properly, along with the tooltips

Note: If the image gallery is tested in Storybook, it will not show the tooltips in the correct location, as we are still dependent on an older version of @wordpress/components when not running in WordPress (where we use whatever version of @wordpress/components it supplies; WordPress 6.1 supplies a version that fixes the tooltip positioning issue).

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.

@mattsherman mattsherman added Enhancement The issue is a request for an enhancement. package: @woocommerce/components issues related to @woocommerce/components focus: components labels Nov 9, 2022
@mattsherman mattsherman requested a review from a team November 9, 2022 22:49
@mattsherman mattsherman self-assigned this Nov 9, 2022
@github-actions
Copy link
Contributor

github-actions bot commented Nov 9, 2022

Test Results Summary

Commit SHA: 4d4e16f

Test 🧪Passed ✅Failed 🚨Broken 🚧Skipped ⏭️Unknown ❔Total 📊Duration ⏱️
API Tests25900302620m 54s
E2E Tests186006019214m 58s
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 approved these changes Nov 14, 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.

Testing well and looks much better! Thanks for this!

Not directly related to this PR and we can handle in a follow-up, but noticed a couple of oddities around the toolbar. In storybook, it's difficult to see the toolbar in the top row:

Screen Shot 2022-11-14 at 9 59 18 AM

The tooltips appear below the image, far from the toolbar itself:

Screen Shot 2022-11-14 at 10 00 31 AM

packages/js/components/src/image-gallery/image-gallery-toolbar.scss
top: -58px;
left: 50%;
transform: translateX(-50%);
z-index: 1000;
Copy link
Contributor

@joshuatf joshuatf Nov 14, 2022

Choose a reason for hiding this comment

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

Is this an arbitrary number or relative to some other component? Just curious if it's relative to some other component; if so we could consider using z-index('.other-component') but not a blocker for this PR.

Copy link
Contributor Author

@mattsherman mattsherman Nov 14, 2022

Choose a reason for hiding this comment

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

Arbitrary. I tried using the approach you mentioned, but it complained that the other component (the image gallery container element) didn't have a z-index. I made a note to circle back to this.

@mattsherman
Copy link
Contributor Author

mattsherman commented Nov 14, 2022

Testing well and looks much better! Thanks for this!

Not directly related to this PR and we can handle in a follow-up, but noticed a couple of oddities around the toolbar. In storybook, it's difficult to see the toolbar in the top row:

Screen Shot 2022-11-14 at 9 59 18 AM

Yeah, that should ideally be improved. I've made a note.

The tooltips appear below the image, far from the toolbar itself:

Screen Shot 2022-11-14 at 10 00 31 AM

Yes, that was mentioned in the PR description...

Note: If the image gallery is tested in Storybook, it will not show the tooltips in the correct location, as we are still dependent on an older version of @wordpress/components when not running in WordPress (where we use whatever version of @wordpress/components it supplies; WordPress 6.1 supplies a version that fixes the tooltip positioning issue).

I'm not sure how to best handle this issue... because if running with an older version of WordPress (and thus @wordpress/components) we will have this bug in the product screen as well.

@mattsherman mattsherman merged commit 932b338 into trunk Nov 14, 2022
@mattsherman mattsherman deleted the fix/product-images-toolbar-positioning branch November 14, 2022 21:08
@github-actions github-actions bot added this to the 7.2.0 milestone Nov 14, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@joshuatf joshuatf joshuatf approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@mattsherman mattsherman

Labels

Enhancement The issue is a request for an enhancement. package: @woocommerce/components issues related to @woocommerce/components

Projects

None yet

Milestone

7.2.0

Development

Successfully merging this pull request may close these issues.

[Enhancement]: Add tooltips for all icons in the image toolbar [Enhancement]: Add space between a selected image in the gallery and the toolbar

3 participants

@mattsherman @joshuatf