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

CCD Merch Card Combined changes #3063

Merged
merged 116 commits into from
Oct 29, 2024
Merged

CCD Merch Card Combined changes #3063

merged 116 commits into from
Oct 29, 2024

Conversation

Axelcureno
Copy link
Member

@Axelcureno Axelcureno commented Oct 17, 2024

@Roycethan Roycethan added verified PR has been E2E tested by a reviewer Ready for Stage labels Oct 24, 2024
@afmicka
Copy link
Contributor

afmicka commented Oct 25, 2024

@Axelcureno @Roycethan in figma, the See terms link is said specifically to be blue. The color is changed to grey in this commit and is left blue for all the other cards. Is there any file/place where this change is documented (need the latest requirements for the automation tests)? cc: @3ch023
Screenshot 2024-10-25 at 11 26 27

In the dev file all the cards are with blue See terms in the screenshots and with text requirements for legal links to be grey (though i believe they meant grey for the line below the links, where prices are).
Screenshot 2024-10-25 at 11 53 27

This either needs to be changed for all the cards or left blue for all the cards. Changing it only for suggested cards seems off to me.

@milo-pr-merge
Copy link
Contributor

milo-pr-merge bot commented Oct 28, 2024

Skipped 3063: "CCD Merch Card Combined changes" due to file "libs/deps/mas/mas.js" overlap. Merging will be attempted in the next batch

@afmicka
Copy link
Contributor

afmicka commented Oct 28, 2024

@Axelcureno also, strikethrough price is lighter grey in the card design in figma files. Not sure it that is something that needs to be accommodated as well
Screenshot 2024-10-28 at 10 31 19

@3ch023
Copy link
Contributor

3ch023 commented Oct 28, 2024

@afmicka would it be ok if we address it in the next PR? this one is blocking another upcoming change that we need soon

@Axelcureno
Copy link
Member Author

@Axelcureno also, strikethrough price is lighter grey in the card design in figma files. Not sure it that is something that needs to be accommodated as well Screenshot 2024-10-28 at 10 31 19

@afmicka from what browser are you seeing this? It looks as expected in Chrome:
Screenshot 2024-10-28 at 7 45 47 AM

@afmicka
Copy link
Contributor

afmicka commented Oct 28, 2024

@3ch023 @Axelcureno
For me the prices are not rendering on CCD page for suggested cards in Safari after the latest changes. Could you please take a look? Removing the label until this is comfirmed.

Screenshot 2024-10-28 at 15 45 10

@afmicka
Copy link
Contributor

afmicka commented Oct 28, 2024

@Axelcureno also, strikethrough price is lighter grey in the card design in figma files. Not sure it that is something that needs to be accommodated as well Screenshot 2024-10-28 at 10 31 19

@afmicka from what browser are you seeing this? It looks as expected in Chrome: Screenshot 2024-10-28 at 7 45 47 AM

@Axelcureno I look the design at figma files in chrome browser. In figma is light colour, in real browser is dark one

  1. https://www.figma.com/design/7tUtNgFelfMjgPoJ5QcE1k/Merch%40Scale-Frameworks?node-id=2034-44369&node-type=frame&t=5QtthpCU9hdUai35-0

  2. https://www.figma.com/proto/7tUtNgFelfMjgPoJ5QcE1k/Merch%40Scale-Frameworks?node-id=2034-44369&t=8hahWyGISCJhs7SZ-1&show-proto-sidebar=1&starting-point-node-id=2034%3A44311

@Axelcureno
Copy link
Member Author

@3ch023 @Axelcureno For me the prices are not rendering on CCD page for suggested cards after the latest changes. Could you please take a look? Removing the label until this is comfirmed.

Screenshot 2024-10-28 at 15 45 10

@afmicka prices render fine for me. Can you share more details of browser and URL? Are you connected to VPN? Please keep in mind only Chrome browser is in scope for this project. I see fonts are also not rendering in your page, that should not be happening.

Screenshot 2024-10-28 at 7 49 22 AM

@afmicka
Copy link
Contributor

afmicka commented Oct 28, 2024

@3ch023 @Axelcureno For me the prices are not rendering on CCD page for suggested cards after the latest changes. Could you please take a look? Removing the label until this is comfirmed.
Screenshot 2024-10-28 at 15 45 10

@afmicka prices render fine for me. Can you share more details of browser and URL? Are you connected to VPN? Please keep in mind only Chrome browser is in scope for this project. I see fonts are also not rendering in your page, that should not be happening.

Screenshot 2024-10-28 at 7 49 22 AM

Sorry, did not finish the message properly before hitting enter.
Safari browser (connected to VPN): https://ccd-axel-changes--milo--adobecom.hlx.page/libs/features/mas/docs/ccd.html

@afmicka
Copy link
Contributor

afmicka commented Oct 28, 2024

Ok, putting back the label.

  1. Strikethrough price color fixed
  2. Safari is not in scope
  3. See terms label color to be addressed in the next PR after clarifying the design for each card type
  4. Fonts are part of other PR not yet merged

Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ [eslint] reported by reviewdog 🐶
File ignored because of a matching ignore pattern. Use "--no-ignore" to override.

@Roycethan
Copy link

Roycethan commented Oct 28, 2024

@Axelcureno Looks like you changed the color for Strikethrough to "Blue" - can you plz check ? Also code coverage ?
image

@milo-pr-merge milo-pr-merge bot merged commit 351c302 into stage Oct 29, 2024
14 checks passed
@milo-pr-merge milo-pr-merge bot deleted the ccd-axel-changes branch October 29, 2024 08:01
@milo-pr-merge milo-pr-merge bot mentioned this pull request Oct 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
commerce merch card Ready for Stage run-nala Run Nala Test Automation against PR verified PR has been E2E tested by a reviewer
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants