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

gc-featured-link: adding bg color option #2214

Merged
merged 1 commit into from
Aug 29, 2023
Merged

Conversation

Garneauma
Copy link
Contributor

Adding option to customize the background colour of featured links.

Added CSS to hide the customized link by default to prevent a colour flick on load.

@Garneauma Garneauma temporarily deployed to github-ci July 20, 2023 18:35 — with GitHub Actions Inactive
Copy link
Collaborator

@GormFrank GormFrank left a comment

Choose a reason for hiding this comment

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

Partial review

components/gc-featured-link/gc-featured-link.js Outdated Show resolved Hide resolved
@Garneauma Garneauma temporarily deployed to github-ci August 7, 2023 16:34 — with GitHub Actions Inactive
@duboisp duboisp self-assigned this Aug 21, 2023
Copy link
Member

@duboisp duboisp left a comment

Choose a reason for hiding this comment

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

  • This is broken in basic HTML mode, we should still display the featured banner. In basic mode I can see a white text with a white background.
  • How do we ensure the text inside the banner do have and maintain a sufficient contrast ratio? Should we auto switch the text color between black and white depending of the luminosity of the background color that was set?

@Garneauma
Copy link
Contributor Author

Garneauma commented Aug 22, 2023

This is broken in basic HTML mode, we should still display the featured banner. In basic mode I can see a white text with a white background.

Adjusted to only add the white background on standard version.

How do we ensure the text inside the banner do have and maintain a sufficient contrast ratio? Should we auto switch the text color between black and white depending of the luminosity of the background color that was set?

If you look at the example, there is the following alert:
"Accessibility consideration
Make sure that the background color has enough contrast with the text as required by WCAG 1.4.3."

Copy link
Member

@duboisp duboisp left a comment

Choose a reason for hiding this comment

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

I love the way it is setup and do self-adjust to ensure it meet the contrast ratio.

components/gc-featured-link/_base.scss Outdated Show resolved Hide resolved
components/gc-featured-link/gc-featured-link-en.html Outdated Show resolved Hide resolved
@Garneauma Garneauma temporarily deployed to github-ci August 23, 2023 12:58 — with GitHub Actions Inactive
Copy link
Member

@duboisp duboisp left a comment

Choose a reason for hiding this comment

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

Testé localement et ça fonctionne tel que prévu, il va rester à adjuster la couleur du texte par défaut juste avant que l'initialization soit complété.

components/gc-featured-link/_base.scss Outdated Show resolved Hide resolved
components/gc-featured-link/_base.scss Outdated Show resolved Hide resolved
Copy link
Member

@duboisp duboisp left a comment

Choose a reason for hiding this comment

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

Pre-approved upon the default loading style in standard mode do have sufficient contrast ratio

@Garneauma Garneauma temporarily deployed to github-ci August 29, 2023 12:21 — with GitHub Actions Inactive
Copy link
Member

@duboisp duboisp left a comment

Choose a reason for hiding this comment

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

Tested locally and reviewed. Every work as expected.

@duboisp duboisp merged commit 3322b19 into wet-boew:master Aug 29, 2023
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants