-
Notifications
You must be signed in to change notification settings - Fork 137
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
Add a product "cover" tag over the cover image #13344
Add a product "cover" tag over the cover image #13344
Conversation
📲 You can test the changes from this Pull Request in WooCommerce-Wear Android by scanning the QR code below to install the corresponding build.
|
📲 You can test the changes from this Pull Request in WooCommerce Android by scanning the QR code below to install the corresponding build.
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## trunk #13344 +/- ##
=========================================
Coverage 41.08% 41.09%
- Complexity 6422 6424 +2
=========================================
Files 1322 1322
Lines 77249 77260 +11
Branches 10660 10660
=========================================
+ Hits 31740 31749 +9
- Misses 42693 42694 +1
- Partials 2816 2817 +1 ☔ View full report in Codecov by Sentry. |
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 looks good, nice work! I tested this in various situations: dark mode, light mode, very big font and display, landscape and portrait, and there's no issue.
The added unit tests I feel are also adequate.
My only slight concern is that with the background color and font color that the "Cover" indicator has, it looks a lot like a button especially on bigger fonts. Note how the bg and text color is similar to the plus button:
light | dark |
---|---|
Lastly, with the addition of this tag, merchants will start having questions about what is "Cover", and how to change it. What do you think about updating the text here to say: "Drag and drop to re-order photos. First photo will be set as the cover." ? I think that will help with that situation (we still have to figure out how to explain what cover is, though).
Hey @hafizrahman thanks for the excellent input.
Yes, that might be a bit confusing, although I don't think is a major issue. Since the
That's an excellent point! Change applied. cc: @itsmeichigo (I'll update the PT with this change, but in the meantime, beware of this Huong). |
Yeah, I think the look is not ideal, but I don't know how to best present it as well. It's not a blocker indeed imo.
Nice! Looks great. |
Closes: #13339
Description
Inspired by the new product editor design from
wc-admin,
we are adding a productCover
label on top of the product image that will be used as the product cover.Main changes happening in:
ProductImagesViewModel
WCProductImageGalleryView
Testing information
Cover
in the image that will be used as the product cover.The tests that have been performed
The above.
Images/gif
AddCoverTag.mp4
RELEASE-NOTES.txt
if necessary. Use the "[Internal]" label for non-user-facing changes.Reviewer (or Author, in the case of optional code reviews):
Please make sure these conditions are met before approving the PR, or request changes if the PR needs improvement: