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

Feature: Color contrast checker #131

Merged
merged 9 commits into from
Oct 30, 2023

Conversation

sushma1031
Copy link
Contributor

@sushma1031 sushma1031 commented Oct 26, 2023

Type of Pull Request

  • Enhancement

Related Issue #s or links (if any):
Fixes #96

Description of Changes

Added a text contrast checking tool in the colors page, similar to the website referenced in the issue. Added the required namespace and tests as well.

@netlify
Copy link

netlify bot commented Oct 26, 2023

Deploy Preview for developertools-tech ready!

Built without sensitive environment variables

Name Link
🔨 Latest commit 9c3254a
🔍 Latest deploy log https://app.netlify.com/sites/developertools-tech/deploys/653fc66f91e0110008f5fd28
😎 Deploy Preview https://deploy-preview-131--developertools-tech.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Copy link
Member

@dlford dlford left a comment

Choose a reason for hiding this comment

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

@sushma1031 great job on this!

Just a couple of adjustments needed:

  • "Normal Text", "Large Text", and "Graphical Objects and User Interface Components" headings should be h3s, not h5s
  • The foreground and background translations strings shouldn't be in all caps
  • The "contrast ratio" label is not translated, also needs to be title cased
  • Can the "swap" button fit in the copy/paste button group to the left of "copy"?
  • The text input field under "Graphical Objects and User Interface Components" doesn't match the selected colors, is this intentional?
  • The value of the contrast ratio should not be an h3 as it is not a heading

@sushma1031
Copy link
Contributor Author

sushma1031 commented Oct 28, 2023

  • I think the swap button can fit on the left, which text field should I add it to?
  • The text input field's border matches the selected foreground colour, same as the referenced website. The colours of the background and text inside the input field do not change.
  • Can we still have the contrast ratio value in a larger size, as in, change it to a p element and increase the font size?

I'll take care of the remaining changes.

@dlford
Copy link
Member

dlford commented Oct 28, 2023

@sushma1031 I'm not sure what you mean by "I think the swap button can fit on the left, which text field should I add it to?"? I mainly just wanted the swap button to not be below the other buttons for that field, does that make sense? Apologies for not being more clear.

The input text makes sense, that can stay as is.

Yes, the contrast text can be larger, the current size is perfectly fine, just the semantics of heading tags does not make sense for that.

Thank you!

@sushma1031
Copy link
Contributor Author

I'm not sure what you mean by "I think the swap button can fit on the left, which text field should I add it to?"? I mainly just wanted the swap button to not be below the other buttons for that field, does that make sense? Apologies for not being more clear.

Oh, right, I understood what you mean. The swap button isn't actually part of the text field, its purpose is to allow the user to swap the background and foreground colors, so I thought placing it in between the fields would be appropriate.
Similar to this:
IMG-20231028-WA0000

@dlford
Copy link
Member

dlford commented Oct 28, 2023

@sushma1031 I see what you mean, maybe it just needs a little more vertical margin to get it away from the other buttons on that field? Whichever you think is best.

@sushma1031
Copy link
Contributor Author

sushma1031 commented Oct 28, 2023

Yes, I agree that a little more vertical margin would be better, I'll add the same.

@sushma1031
Copy link
Contributor Author

"Normal Text", "Large Text", and "Graphical Objects and User Interface Components" headings should be h3s, not h5s

image

@dlford I made the mentioned headings h3, and I think they look too large. Can we reduce the font size?

@sushma1031
Copy link
Contributor Author

maybe it just needs a little more vertical margin to get it away from the other buttons on that field? Whichever you think is best.

image

Is this much vertical margin sufficient?

@dlford
Copy link
Member

dlford commented Oct 30, 2023

@sushma1031 that vertical margin looks good!

For the h3s, yes please make them smaller, thank you!

@sushma1031
Copy link
Contributor Author

@dlford I made the requested changes, let me know if there's anything else!

@dlford
Copy link
Member

dlford commented Oct 30, 2023

@sushma1031 looks great, thank you!

@dlford dlford added the hacktoberfest-accepted This PR will be counted for Hacktoberfest label Oct 30, 2023
@dlford dlford merged commit a3bd8e5 into developertools-tech:dev Oct 30, 2023
5 checks passed
@sushma1031 sushma1031 deleted the color-contrast-checker branch October 31, 2023 01:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
hacktoberfest-accepted This PR will be counted for Hacktoberfest
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants