Skip to content
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

MWPW-152160 and MWPW-152155 card widths and gray divider #3382

Open
wants to merge 11 commits into
base: stage
Choose a base branch
from

Conversation

bozojovicic
Copy link
Contributor

@bozojovicic bozojovicic commented Dec 16, 2024

Image and product cards should be 276px wide when displayed in 4 columns, 378px when displayed in 3 or less columns.
This makes sense only for screen wider than 1200px. That is not AC but I need to discuss it with the reporter of that ticket.
Also the gray divider on product card has to have 16px top margin.
Tested on kitchen sink page.

Test page https://mwpw152160consonant--milo--bozojovicic.aem.page/merch/kitchen-sink?martech=off

Resolves: MWPW-152160
Resolves: MWPW-152155

Test URLs:

Copy link
Contributor

aem-code-sync bot commented Dec 16, 2024

Copy link

codecov bot commented Dec 16, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 96.46%. Comparing base (c582faa) to head (a4864fe).

Additional details and impacted files
@@           Coverage Diff           @@
##            stage    #3382   +/-   ##
=======================================
  Coverage   96.45%   96.46%           
=======================================
  Files         255      255           
  Lines       59420    59415    -5     
=======================================
  Hits        57314    57314           
+ Misses       2106     2101    -5     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@bozojovicic bozojovicic marked this pull request as ready for review December 17, 2024 09:29
libs/features/mas/src/global.css.js Outdated Show resolved Hide resolved
libs/features/mas/src/variants/image.css.js Show resolved Hide resolved
Copy link
Contributor

@3ch023 3ch023 left a comment

Choose a reason for hiding this comment

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

PR changes are good but I have an extra request, sorry :)

Until now were using https://main--milo--adobecom.hlx.page/docs/library/kitchen-sink/merch-card for all merch card variations, but i recently discovered this page should only be owned by GWP.

this means we as a dev team are missing a gallery page with all acom merch-card variations.
Could you pls work with @afmicka or @Roycethan and create one?
You can chose some 'stable' location in Milo and lets document it (maybe in readme of /libs/features/mas or announce in our channel)
this gallery should have the examples of the product and image cards you fixed in this pr

@bozojovicic
Copy link
Contributor Author

PR changes are good but I have an extra request, sorry :)

OK @3ch023 I will ask @Roycethan if he can do it. I tried to do it but for some documents I don't have rights not even to copy them from folder A to B.

@Roycethan
Copy link

Roycethan commented Dec 23, 2024

PR changes are good but I have an extra request, sorry :)

OK @3ch023 I will ask @Roycethan if he can do it. I tried to do it but for some documents I don't have rights not even to copy them from folder A to B.

Created here : https://main--milo--adobecom.hlx.page/merch/kitchen-sink
Added 4 column fragments
and your version of fix can be viewed here:
and your version:
https://mwpw152160consonant--milo--bozojovicic.aem.page/merch/kitchen-sink

Copy link
Contributor

This pull request is not passing all required checks. Please see this discussion for information on how to get all checks passing. Inconsistent checks can be manually retried. If a test absolutely can not pass for a good reason, please add a comment with an explanation to the PR.

Copy link
Contributor

github-actions bot commented Jan 1, 2025

This PR has not been updated recently and will be closed in 7 days if no action is taken. Please ensure all checks are passing, https://github.com/orgs/adobecom/discussions/997 provides instructions. If the PR is ready to be merged, please mark it with the "Ready for Stage" label.

@github-actions github-actions bot added the Stale label Jan 1, 2025
@github-actions github-actions bot removed the Stale label Jan 4, 2025
@narcis-radu
Copy link
Contributor

@bozojovicic - please update the test link in your description, it will be easier to understand the changes.

@narcis-radu narcis-radu self-requested a review January 10, 2025 13:04
@afmicka
Copy link
Contributor

afmicka commented Jan 13, 2025

@bozojovicic
I see few issues with the product card.

  1. Per figma, the spacing between the grey line and anything above and below should be 16px. On the card with the callout text, that spacing is much bigger (notice the gaps). There is an example in figma with no gap, pasting here screenshots for comparison.

  2. I have added second CTA to the product card, that CTA does NOT show on the card when width 276px. It shows when the card has width of 378px, but still now per design. Secure transaction should go above CTAs. The same screenshots for comparison how the card should look when there are 2 ctas.

Your branch:
Screenshot 2025-01-13 at 13 15 49
Screenshot 2025-01-13 at 13 25 38

Figma 276px:
Screenshot 2025-01-13 at 13 26 56

Figma 378px:
Screenshot 2025-01-13 at 13 32 54

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants