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

AC-2484::Contrast insufficient - carousel navigation buttons (Landing… #3787

Merged
merged 4 commits into from
Jul 18, 2022

Conversation

glo71317
Copy link
Collaborator

@glo71317 glo71317 commented Apr 26, 2022

Description

Contrast insufficient - carousel navigation buttons (Landing Page)

Reproduction Steps
[NODE][body>div:nth-of-type(1)>:nth-child(1)>:nth-child(3)]

  1. Use a color contrast checker to compare foreground and background colors.

Actual Behavior
Navigation buttons for carousels do not meet minimum contrast requirements, depending on their current state.

Examples include:
Homepage hero carousel, dot icon buttons for the non-current slides:
Foreground:#DFE1E2
Background:#F3F3F3
Contrast ratio: 1.2:1

"Top Sellers" carousel, slider-style buttons for non-current set of items:
Foreground:#DFE1E2
Background:#FFFFFF
Contrast ratio: 1.3:1

When sufficient contrast ratios are not met, users with low vision may have trouble identifying user interface components.

Expected Behavior
Provide identifying portions of buttons, form fields, and other user interface controls a contrast ratio of at least 3:1 with adjacent colors. This may be via the interior or exterior colors for borders but does not require both. In addition, ensure that states such as focus, hover, selected, current item, and other states, like checked, also provide sufficient contrast to identify that state with adjacent colors.

Use the Level Access contrast checking tool or plugin (https://accessibility.dev/) to determine if the contrast is sufficient.

Related Issue

Closes https://jira.corp.magento.com/browse/AC-2484.

Acceptance

Verification Stakeholders

Specification

Verification Steps

Pre-Conditions:

  1. Have Magento instance with sample data installed
  2. Make sure to have pwa studio installed
  3. Make sure to have a customer login for front end login

Manual Steps executed:

Login to venia > Navigate to global header venia and press tab to navigate to carousel button
use color contrast checker to compare foreground and background colors.

✖️ Behaviour Before The Fix : Navigation buttons for carousels do not meet minimum contrast requirements of 3:1 with adjacent colors
image

✔️Behaviour After The Fix: Navigation buttons for carousels meet minimum contrast requirements of 3:1 with adjacent colors
image

Breaking Changes (if any)

Checklist

  • I have added tests to cover my changes, if necessary.
  • I have added translations for new strings, if necessary.
  • I have updated the documentation accordingly, if necessary.

Resolved issues:

  1. resolves [Issue] AC-2484::Contrast insufficient - carousel navigation buttons (Landing… #3836: AC-2484::Contrast insufficient - carousel navigation buttons (Landing…

@pwa-studio-bot
Copy link
Collaborator

pwa-studio-bot commented Apr 26, 2022

Messages
📖

Associated JIRA tickets: AC-2484.

📖 DangerCI Failures related to missing labels/description/linked issues/etc will persist until the next push or next pr-test build run (assuming they are fixed).
📖

Access a deployed version of this PR here. Make sure to wait for the "pwa-pull-request-deploy" job to complete.

Generated by 🚫 dangerJS against 37f8ce8

@glo71317
Copy link
Collaborator Author

glo71317 commented May 2, 2022

run all tests

@supernova-at
Copy link
Contributor

@magento create issue from PR

@ericeoeur
Copy link
Contributor

Hi @glo71317, thank you for your work on this. There are three items that need to be addressed before this passes review:

  • A version label is required for this PR, I believe this would be patch.
  • Cypress tests snapshots for products.specs.js must be updated to reflect the color contrast changes in carousel navigation buttons
  • Based on the this PR's deployed instance, The "Top Sellers" carousel (slider-style buttons for non-current set of items) is not reflective of the fixed contrast changes as shown in the Homepage hero carousel (dot icon buttons for the non-current slides) . Should this fix be included since it was mentioned in the reproduction steps? See the image below for details:
    image

@glo82145
Copy link
Collaborator

run all tests

@glo82145
Copy link
Collaborator

Hi @glo71317, thank you for your work on this. There are three items that need to be addressed before this passes review:

  • A version label is required for this PR, I believe this would be patch.
  • Cypress tests snapshots for products.specs.js must be updated to reflect the color contrast changes in carousel navigation buttons
  • Based on the this PR's deployed instance, The "Top Sellers" carousel (slider-style buttons for non-current set of items) is not reflective of the fixed contrast changes as shown in the Homepage hero carousel (dot icon buttons for the non-current slides) . Should this fix be included since it was mentioned in the reproduction steps? See the image below for details:
    image

@ericeoeur .. We have added the contrast for both the slider on home page .. please have a look and let us know

@ericeoeur
Copy link
Contributor

run all tests

@pwa-test-bot
Copy link

pwa-test-bot bot commented May 25, 2022

Successfully started codebuild job for the following test suites:

lighthouse-desktop
lighthouse-mobile
pr-test
scaffold-pwa
cypress

@pwa-studio-bot
Copy link
Collaborator

pwa-studio-bot commented May 25, 2022

Fails
🚫

node failed.

🚫

Cypress tests in the following files did not pass 😔. All tests must pass before this PR can be merged

  • user can fill and submit payment form and place order with default badge position:
  • should display French text and EUR currency accross app if French Store View is selected:
    should show USD currency across app if it is selected inside French Store View:

Log

ERROR ON TASK: cypressTests


Error:  Danger had errors running. See message(s) above for more details.
danger-results://tmp/danger-results.json

Generated by 🚫 dangerJS against 37f8ce8

@ericeoeur ericeoeur added the version: Patch This changeset includes backwards compatible bug fixes. label May 31, 2022
@ericeoeur
Copy link
Contributor

run all tests

@pwa-test-bot
Copy link

pwa-test-bot bot commented May 31, 2022

Successfully started codebuild job for the following test suites:

scaffold-pwa
cypress
lighthouse-desktop
lighthouse-mobile
pr-test

@jcalcaben
Copy link
Contributor

run all tests

@pwa-test-bot
Copy link

pwa-test-bot bot commented Jul 13, 2022

Successfully started codebuild job for the following test suites:

scaffold-pwa
cypress
lighthouse-desktop
lighthouse-mobile
pr-test

@pwa-studio-bot
Copy link
Collaborator

Fails
🚫

node failed.

Log

Error: Error: Cannot find module './.lighthouseci/assertion-results.json'
Require stack:
- dangerfile.lighthouse.js
- /usr/local/share/.config/yarn/global/node_modules/danger/distribution/runner/runners/inline.js
- /usr/local/share/.config/yarn/global/node_modules/danger/distribution/commands/danger-runner.js

ERROR ON TASK: lighthouseTests


Error:  Danger had errors running. See message(s) above for more details.
danger-results://tmp/danger-results.json

Generated by 🚫 dangerJS against 75590f1

@pwa-studio-bot
Copy link
Collaborator

Fails
🚫

node failed.

Log

Error: Error: Cannot find module './.lighthouseci/assertion-results.json'
Require stack:
- dangerfile.lighthouse.js
- /usr/local/share/.config/yarn/global/node_modules/danger/distribution/runner/runners/inline.js
- /usr/local/share/.config/yarn/global/node_modules/danger/distribution/commands/danger-runner.js

ERROR ON TASK: lighthouseTests


Error:  Danger had errors running. See message(s) above for more details.
danger-results://tmp/danger-results.json

Generated by 🚫 dangerJS against 75590f1

jcalcaben
jcalcaben previously approved these changes Jul 14, 2022
@dpatil-magento
Copy link
Contributor

run pr-deploy

@pwa-test-bot
Copy link

pwa-test-bot bot commented Jul 15, 2022

Successfully started codebuild job for pr-deploy

@dpatil-magento
Copy link
Contributor

run cypress

@pwa-test-bot
Copy link

pwa-test-bot bot commented Jul 15, 2022

Successfully started codebuild job for cypress

@dpatil-magento dpatil-magento changed the base branch from GL-web-accessibility to develop July 15, 2022 18:45
@dpatil-magento dpatil-magento dismissed jcalcaben’s stale review July 15, 2022 18:45

The base branch was changed.

@dpatil-magento
Copy link
Contributor

run cypress

@pwa-test-bot
Copy link

pwa-test-bot bot commented Jul 15, 2022

Successfully started codebuild job for cypress

@dpatil-magento
Copy link
Contributor

run cypress

@pwa-test-bot
Copy link

pwa-test-bot bot commented Jul 15, 2022

Successfully started codebuild job for cypress

@dpatil-magento
Copy link
Contributor

QA approved, failed cypress are not related.

@dpatil-magento dpatil-magento merged commit 7eced81 into magento:develop Jul 18, 2022
@m2-community-project m2-community-project bot moved this from Reviewer Approved to Done in Pull Request Progress Jul 18, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pkg:venia-ui Progress: done version: Patch This changeset includes backwards compatible bug fixes.
Development

Successfully merging this pull request may close these issues.

[Issue] AC-2484::Contrast insufficient - carousel navigation buttons (Landing…
7 participants