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

Various catalog fixes + SWC updates #2347

Merged
merged 35 commits into from
May 30, 2024
Merged

Various catalog fixes + SWC updates #2347

merged 35 commits into from
May 30, 2024

Conversation

@yesil yesil requested review from a team as code owners May 21, 2024 13:05
Copy link
Contributor

aem-code-sync bot commented May 21, 2024

Hello, I'm the AEM Code Sync Bot and I will run some actions to deploy your branch and validate page speed.
In case there are problems, just click a checkbox below to rerun the respective action.

  • Re-run PSI checks
  • Re-sync branch
Commits

@yesil yesil added high-impact Any PR that may affect consumers run-nala Run Nala Test Automation against PR commerce labels May 21, 2024
Copy link
Contributor

aem-code-sync bot commented May 21, 2024

Page Scores Audits Google
/drafts/ilyas/merch-card-collection?martech=off PERFORMANCE A11Y SEO BEST PRACTICES SI FCP LCP TBT CLS PSI

Copy link

codecov bot commented May 21, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 95.66%. Comparing base (29d0a16) to head (1227eb3).

Additional details and impacted files
@@           Coverage Diff           @@
##            stage    #2347   +/-   ##
=======================================
  Coverage   95.65%   95.66%           
=======================================
  Files         173      173           
  Lines       45165    45176   +11     
=======================================
+ Hits        43204    43216   +12     
+ Misses       1961     1960    -1     

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

Copy link
Contributor

@npeltier npeltier left a comment

Choose a reason for hiding this comment

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

lgtm assuming no regression :)

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.

when the icon is missing the card layout gets broken
image

@3ch023
Copy link
Contributor

3ch023 commented May 21, 2024

also on kitchen sync there is a trouble with the card title
https://product-catalog-fixes--milo--adobecom.hlx.page/docs/library/kitchen-sink/merch-card
image

@skumar09 skumar09 added run-nala Run Nala Test Automation against PR and removed run-nala Run Nala Test Automation against PR labels May 21, 2024
@aem-code-sync aem-code-sync bot temporarily deployed to product-catalog-fixes May 21, 2024 14:35 Inactive
@aem-code-sync aem-code-sync bot temporarily deployed to product-catalog-fixes May 21, 2024 14:43 Inactive
@aem-code-sync aem-code-sync bot temporarily deployed to product-catalog-fixes May 21, 2024 14:52 Inactive
@aem-code-sync aem-code-sync bot temporarily deployed to product-catalog-fixes May 21, 2024 19:19 Inactive
@yesil
Copy link
Contributor Author

yesil commented May 21, 2024

@3ch023 the icon issue is not a product of this PR. the card collection index used in the Milo page is hand crafted and not entirely correct. Any style issue should be reported with examples in kitchen sink or other pages.

@Roycethan
Copy link

@yesil Tickets attached to this PR are verified, they are fixed. I see a other issue and tracked here https://jira.corp.adobe.com/browse/MWPW-150558 plz let me know whether you are planning to fix it here or in another PR, thanks

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.

@yesil
Copy link
Contributor Author

yesil commented May 22, 2024

@3ch023 could you please review again?

@yesil
Copy link
Contributor Author

yesil commented May 29, 2024

@Roycethan @afmicka as per the design: https://xd.adobe.com/view/84200b4e-caf9-428f-98cf-7198a236a4bc-d173/screen/9e57473d-b3de-4ad1-8627-ca485d21871c/variables/

the tablet should show 3 cards in the same row and buttons must be stretched.

Let me know if you disagree.

@aem-code-sync aem-code-sync bot temporarily deployed to product-catalog-fixes May 29, 2024 14:14 Inactive
@aem-code-sync aem-code-sync bot temporarily deployed to product-catalog-fixes May 29, 2024 14:33 Inactive
@aem-code-sync aem-code-sync bot temporarily deployed to product-catalog-fixes May 29, 2024 16:35 Inactive
@aem-code-sync aem-code-sync bot temporarily deployed to product-catalog-fixes May 29, 2024 16:43 Inactive
@Roycethan
Copy link

Roycethan commented May 29, 2024

@yesil Thanks for fixing regressions in Mini compare chart card, few issues/regressions in Catalog page mentioning here:

  1. CTA misalignment: https://catalog-fixes--cc--adobecom.hlx.live/products/catalog?martech=off&milolibs=product-catalog-fixes#category=pdf-esignatures
image image
  1. Some more device issues are logged separately fyi and can be handled in post PRs:
    https://jira.corp.adobe.com/browse/MWPW-151437
    https://jira.corp.adobe.com/browse/MWPW-151392
    https://jira.corp.adobe.com/browse/MWPW-150558

cc @afmicka

@aem-code-sync aem-code-sync bot temporarily deployed to product-catalog-fixes May 30, 2024 06:18 Inactive
@aem-code-sync aem-code-sync bot temporarily deployed to product-catalog-fixes May 30, 2024 09:10 Inactive
@aem-code-sync aem-code-sync bot temporarily deployed to product-catalog-fixes May 30, 2024 09:44 Inactive
@afmicka afmicka added run-nala Run Nala Test Automation against PR and removed run-nala Run Nala Test Automation against PR labels May 30, 2024
@afmicka
Copy link
Contributor

afmicka commented May 30, 2024

@yesil there is one remaining gap to be fixed. Tablet view with 2 columns. The gap is still smaller with your changes.
https://main--cc--adobecom.hlx.live/creativecloud?milolibs=product-catalog-fixes&martech=off
Screenshot 2024-05-30 at 12 43 10

xd: https://xd.adobe.com/view/84200b4e-caf9-428f-98cf-7198a236a4bc-d173/screen/7c930c0a-df97-4206-a944-43f1ad98d731/variables/

Could you please also check the font size for the CTAs? It is smaller with your changes (15px) than on prod (17px) but not sure out of XD what value it should really be.
https://main--cc--adobecom.hlx.live/pl/creativecloud?milolibs=product-catalog-fixes&martech=off#
Screenshot 2024-05-30 at 12 52 49

  1. I still see this CTA alignment on the card on desktop view as well, not just mobile. Catalog page.
Screenshot 2024-05-30 at 13 25 03

cc: @Roycethan

@aem-code-sync aem-code-sync bot temporarily deployed to product-catalog-fixes May 30, 2024 12:12 Inactive
@Blainegunn Blainegunn merged commit b128b82 into stage May 30, 2024
10 of 11 checks passed
@Blainegunn Blainegunn deleted the product-catalog-fixes branch May 30, 2024 16:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
commerce high-impact Any PR that may affect consumers run-nala Run Nala Test Automation against PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants