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-161865: adds alt text attribute to background images of merch cards #3272

Closed
wants to merge 30 commits into from

Conversation

Axelcureno
Copy link
Member

@Axelcureno Axelcureno commented Nov 27, 2024

npeltier and others added 17 commits November 15, 2024 09:34
* MWPW-159374: Update hydrate logic

to support all cta styles
organise code and code coverage

Update tests

* add workaround for ccd-suggested cards

* remove source maps

* improve code coverage

* merge MWPW-159374

* MWPW-161355: Update MAS documentation

* Example with contextual menu
* Checkout click event with correct event target
 when the CTA contains a text wrapped by a span

* cleanup all attributes during card hydration

* remove deps/mas/mas.js

* update test

* update doc

* allow list libs/features/mas/mas/dist/mas.js

* change order in ignore

* trying with wildcard

* allow list dist

* update hlxignore rules

* update hlxignore rules

* MWPW-161176: restructure ccd gallery (#3169)

restructure ccd gallery

* fix mas path

* fix undefined error in merch-icon

* fix regression in ctas size

* update doc

* fix css with checkout-link

* fix random rtl issue

* revert removal of mas.js from deps

* update ccd gallery

---------

Co-authored-by: Mariia Lukianets <lukianet@adobe.com>
* MWPW-161845: add analytics

* fix review changes

* add build files
* initial freyja commit

* increase codecov

* trival

* add qa support to ccd page

* improve page performance

* update ccd.html
Refactor merch-card styles to improve structure and specificity
…3196)

* Added space after price recurrence label for display-per-unit prices

* more specific selector

* built

* fix cr

* built
* MWPW-162933: merge mas modules

+ consonant-templates module from tacocat.js
into a single module
* MWPW-160755 - add tests for CCD cards

* refactor and prepare dark mode checks

* add tests for slice cards

* activate dark for suggested and eslint fixes

* add check for cta variant

* fix multi mnemonic check

* fix lint

* add analytics check

* remove comment
Copy link
Contributor

aem-code-sync bot commented Nov 27, 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

Copy link
Contributor

aem-code-sync bot commented Nov 27, 2024

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.

Copy link

codecov bot commented Nov 27, 2024

Codecov Report

Attention: Patch coverage is 98.82199% with 18 lines in your changes missing coverage. Please review.

Project coverage is 96.44%. Comparing base (9884681) to head (dd05ab2).
Report is 16 commits behind head on stage.

Files with missing lines Patch % Lines
libs/features/mas/src/log.js 92.79% 8 Missing ⚠️
libs/features/mas/src/lana.js 95.37% 5 Missing ⚠️
libs/features/mas/src/price/utilities.js 99.27% 2 Missing ⚠️
libs/blocks/merch-card/merch-card.js 0.00% 1 Missing ⚠️
libs/features/mas/src/price/template.js 99.76% 1 Missing ⚠️
libs/features/mas/src/variants/product.js 85.71% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##            stage    #3272      +/-   ##
==========================================
- Coverage   98.84%   96.44%   -2.40%     
==========================================
  Files          70      253     +183     
  Lines        8660    58735   +50075     
==========================================
+ Hits         8560    56648   +48088     
- Misses        100     2087    +1987     

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

afmicka and others added 2 commits November 28, 2024 11:18
* MWPW-163041: fix analytics & add docs

* add tests for analytics

* add analytics docu

* fix test
* bump to 10 seconds

* new change
}
}
if (fields.backgroundImage) {
switch (variant) {
Copy link
Contributor

@3ch023 3ch023 Nov 29, 2024

Choose a reason for hiding this comment

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

i suggest to base the decision on backgroundImageConfig.

if (backgroundImageConfig.tag) {
    // create tag
}
if (backgroundImageConfig.attribute) {
    merchCard.setAttribute(backgroundImageConfig.attribute, fields.backgroundImage);
}

then you only need to add this to ccd-suggested.js AEM_FRAGMENT_MAPPING:

backgroundImage: { attribute: 'background-image' },

this shifts control to the specific variant, leaving hydrate simple and unaware of ccd or adobe home or any other consumer.

Copy link
Member Author

Choose a reason for hiding this comment

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

That's actually a great suggestion. I've made the change and the function is now variant-agnostic. I've also updated UTs accordingly. Thx.

loading: 'lazy',
src: fields.backgroundImage,
};
if (fields.backgroundImageAltText) {
Copy link
Contributor

Choose a reason for hiding this comment

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

jira mentions that if backgroundImageAltText is missing we still need to set role="none" attribute

Ensure elements have alternate text or a role of none or presentation

https://developer.mozilla.org/en-US/docs/Web/Accessibility/ARIA/Roles

Copy link
Member Author

Choose a reason for hiding this comment

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

RIght, added.

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.

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.

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.

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.

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.

@Axelcureno Axelcureno requested a review from 3ch023 December 2, 2024 23:22
yesil and others added 4 commits December 3, 2024 16:54
* MWPW-161645: lana logging for CCD

with <mas-commerce-service data-lana-tags="consumer=ccd" ...>

lana will be enabled.

TODO:

Update docs

* Update tests

* fix review comments

* add host env attribute, fix lana override

* fix docs

* fix unit tests

* start loggin missing osi's

* fix message

* fix message

* Update libs/features/mas/src/lana.js

Co-authored-by: Axel Cureno Basurto <axelcureno@gmail.com>

* PR review changes

* avoid duplicate logging, log wcs url, limit page length in log

* fix formatString error

* fix render

* add tests

* fix loggin

* revert ccd comments

* fix milo tags

---------

Co-authored-by: Mariia Lukianets <mariia.lukianets@gmail.com>
Co-authored-by: Axel Cureno Basurto <axelcureno@gmail.com>
@Roycethan
Copy link

@Axelcureno

  1. Plz resolve js conflicts.
  2. I get this error when saving the alt-text some times-
    https://main--mas--adobecom.hlx.live/studio.html?milolibs=MWPW-161865--milo--adobecom#path=ccd
image

cc @yesil @3ch023 @npeltier @afmicka

@Axelcureno
Copy link
Member Author

@Axelcureno

  1. Plz resolve js conflicts.
  2. I get this error when saving the alt-text some times-
    https://main--mas--adobecom.hlx.live/studio.html?milolibs=MWPW-161865--milo--adobecom#path=ccd
image cc @yesil @3ch023 @npeltier @afmicka

Fixed @Roycethan , please retest.

@Roycethan Roycethan added the verified PR has been E2E tested by a reviewer label Dec 10, 2024
Base automatically changed from ccd to stage December 10, 2024 10:14
@overmyheadandbody overmyheadandbody requested a review from a team as a code owner December 10, 2024 10:14
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 shows 278 changes, you branch needs to be fixed.
will DM in slack

@Axelcureno
Copy link
Member Author

Closing this one, new PR with these changes: #3360

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
commerce merch card verified PR has been E2E tested by a reviewer
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants