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

🖍 [Story interactive] Color Logic for A/B/C/D Bubbles for Image Quizzes #35174

Merged
merged 134 commits into from Jul 19, 2021

Conversation

Brandons42
Copy link
Contributor

@Brandons42 Brandons42 commented Jul 9, 2021

For A/B/C/D bubbles on image quizzes: Set the background color to the accent color (can be customized by user) and automatically set the text color to be a readable color based on the background color

Demo: https://interactive-img.web.app/color-logic/quiz.html

Apologies for messy commit history, only the last 5 commits are relevant (as of the initial PR)

Closes #35183

/cc @mszylkowski
/cc @processprocess

@Brandons42 Brandons42 marked this pull request as ready for review July 9, 2021 21:00
@amp-owners-bot amp-owners-bot bot requested a review from newmuis July 9, 2021 21:00
@amp-owners-bot
Copy link

amp-owners-bot bot commented Jul 9, 2021

Hey @gmajoulet, @mszylkowski! These files were changed:

extensions/amp-story-interactive/0.1/amp-story-interactive-img-quiz.css
extensions/amp-story-interactive/0.1/amp-story-interactive-img-quiz.js

@mszylkowski mszylkowski requested review from processprocess and mszylkowski and removed request for newmuis July 12, 2021 13:50
@mszylkowski
Copy link
Contributor

Just a thought: IMO when the background colors is close to halfway in the brightness spectrum, the text color is easier to read when it matches the theme of the component. However, logic wise I don't think we have a way to make this work, since the contrast checker prefers the black text to the white text almost by 2x the contrast (link) and the logic theoretically should have nothing to do with the background of other parts of the UI that are not directly behind the text anyways.

TL;DR: There are some cases that matching the font color to the theme might make the text more readable, but I'm not sure there's much we can do since the contrast values in the example favors black text.

GoodHarder to readGood

Copy link
Contributor

@mszylkowski mszylkowski left a comment

Choose a reason for hiding this comment

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

Good use of the CSS variables, left some nits

Comment on lines 50 to 55
content: '' !important;
display: block !important;
width: 100% !important;
height: 100% !important;
border-radius: 50% !important;
position: absolute !important;
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: Do we need this change? I'd think it's better the other way (and also, it's not related to the ABCD coloring)

* @param {!Element} root
* @private
*/
setBubbleColor_(root) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: setBubbleTextColor_ helps understand that you're just changing the text color and not the background color

@Brandons42
Copy link
Contributor Author

Just a thought: IMO when the background colors is close to halfway in the brightness spectrum, the text color is easier to read when it matches the theme of the component

I definitely agree

@mszylkowski mszylkowski added this to In progress in wg-stories Sprint via automation Jul 19, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Development

Successfully merging this pull request may close these issues.

[Story interactive] Use contrast color logic for the ABCD on image quizzes
4 participants