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-164131: Adding aria label for CTA using placeholder on name attributes of merch card #3505

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

Conversation

@seanchoi-dev seanchoi-dev requested a review from a team as a code owner January 16, 2025 23:32
Copy link
Contributor

aem-code-sync bot commented Jan 16, 2025

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

Copy link
Contributor

aem-code-sync bot commented Jan 16, 2025

@seanchoi-dev seanchoi-dev requested a review from 3ch023 January 16, 2025 23:35
@seanchoi-dev seanchoi-dev added the run-nala Run Nala Test Automation against PR label Jan 16, 2025
@seanchoi-dev seanchoi-dev changed the title MWPW-164131: Adding aria label for CTA using placeholder on name attributes of the… MWPW-164131: Adding aria label for CTA using placeholder on name attributes of merch card Jan 16, 2025
Copy link

codecov bot commented Jan 16, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 96.48%. Comparing base (a4101ed) to head (69e30d6).
Report is 12 commits behind head on stage.

Additional details and impacted files
@@            Coverage Diff             @@
##            stage    #3505      +/-   ##
==========================================
+ Coverage   96.47%   96.48%   +0.01%     
==========================================
  Files         260      260              
  Lines       60415    60597     +182     
==========================================
+ Hits        58284    58466     +182     
  Misses       2131     2131              

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

@seanchoi-dev seanchoi-dev marked this pull request as draft January 17, 2025 17:41
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

libs/blocks/merch-card/merch-card.js Outdated Show resolved Hide resolved
@seanchoi-dev seanchoi-dev marked this pull request as ready for review January 22, 2025 17:39
@seanchoi-dev seanchoi-dev marked this pull request as draft January 22, 2025 17:40
@seanchoi-dev seanchoi-dev marked this pull request as ready for review January 22, 2025 17:45
@seanchoi-dev seanchoi-dev marked this pull request as draft January 22, 2025 22:34
@seanchoi-dev seanchoi-dev marked this pull request as ready for review January 23, 2025 21:02
@adobecom adobecom deleted a comment from github-actions bot Jan 23, 2025
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.

I think this logic should be in merch block (merch.js), not in merch-card block.
there are Merch CTAs outside of merch-cards and we could set a aria-label for them authomatically which means less authoring effort.

you could add it in the end of buildCTA method https://github.com/adobecom/milo/blob/stage/libs/blocks/merch/merch.js#L723
you can use 'onceSettled' promise on merch link, e.g. cta.onceSettled().then(...set aria-label);

we could and probably should have done it even deeper, in checkout-link/checkout-button web component, but imo for now Milo level of merch block is good enough.

@seanchoi-dev seanchoi-dev requested a review from 3ch023 January 24, 2025 17:47
@seanchoi-dev seanchoi-dev removed the run-nala Run Nala Test Automation against PR label Jan 24, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants