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

feat: add checkbox #140

Open
wants to merge 4 commits into
base: dev
Choose a base branch
from
Open

feat: add checkbox #140

wants to merge 4 commits into from

Conversation

chosww
Copy link
Contributor

@chosww chosww commented Jan 24, 2025

To test, add a checkbox with the checkbox input on a template:

<input type="checkbox" id="id" name="name">

@chosww chosww self-assigned this Jan 24, 2025
@chosww chosww added the enhancement New feature or request label Jan 24, 2025
@chosww chosww linked an issue Jan 24, 2025 that may be closed by this pull request
@chosww chosww requested a review from greatislander January 24, 2025 17:33
@greatislander greatislander enabled auto-merge (squash) January 24, 2025 18:49
@greatislander greatislander added this to the v1.1.0 milestone Jan 24, 2025
Copy link
Member

@greatislander greatislander left a comment

Choose a reason for hiding this comment

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

A couple comments on the approach. Also there are significant visual differences; default state, hover state and focus state don't match border weight in the design, active design background colour does not match the design, check mark position and shape doesn't match the design.

src/_includes/partials/components/checkbox.macro.njk Outdated Show resolved Hide resolved
src/assets/styles/components/_checkbox.css Outdated Show resolved Hide resolved
@chosww
Copy link
Contributor Author

chosww commented Jan 24, 2025

A couple comments on the approach. Also there are significant visual differences; default state, hover state and focus state don't match border weight in the design, active design background colour does not match the design, check mark position and shape doesn't match the design.

Thanks for your review @greatislander. I asked Uttara earlier today about the active design background colour and checkmark colour and Uttara told me that they should be indigo-700. In regards to the check mark position and shape, I don't have control over how it is shown as it is rendered by css content property -- I tried to use the given SVG file for the checkmark using background-image properly, but then the position of the background-image isn't working exactly the way we want in Figma. I think making custom component would fix all the issues, so should I go ahead and create a custom component?

@greatislander
Copy link
Member

A couple comments on the approach. Also there are significant visual differences; default state, hover state and focus state don't match border weight in the design, active design background colour does not match the design, check mark position and shape doesn't match the design.

Thanks for your review @greatislander. I asked Uttara earlier today about the active design background colour and checkmark colour and Uttara told me that they should be indigo-700. In regards to the check mark position and shape, I don't have control over how it is shown as it is rendered by css content property -- I tried to use the given SVG file for the checkmark using background-image properly, but then the position of the background-image isn't working exactly the way we want in Figma. I think making custom component would fix all the issues, so should I go ahead and create a custom component?

The best way to do this is to use the mask-image property with an SVG URL. Let's pair up to look at this when you have some time.

@greatislander
Copy link
Member

@chosww In case I am not available to talk through this with you tomorrow, here is the example I put together: https://codepen.io/greatislander/pen/dPbwzjw

Copy link

cloudflare-workers-and-pages bot commented Jan 28, 2025

Deploying inclusive-standards with  Cloudflare Pages  Cloudflare Pages

Latest commit: 92bc8c9
Status: ✅  Deploy successful!
Preview URL: https://df98e099.inclusive-standards.pages.dev
Branch Preview URL: https://feat-checkbox.inclusive-standards.pages.dev

View logs

@chosww chosww requested a review from greatislander January 28, 2025 16:29
@greatislander greatislander requested a review from jobara January 28, 2025 16:48
Copy link
Member

@greatislander greatislander left a comment

Choose a reason for hiding this comment

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

  • Update colours so that the checkboxes appear properly on indigo-200 backgrounds (waiting on @uttaraghodke to update the Figma design)
  • Use inset box-shadow instead of outline for more consistent dimensions in inactive and hover states
  • Set transparent outline for focus state to preserve Windows High Contrast Mode behaviour
  • Style parent nodes of checkbox inputs to use flex display with align-items center and an appropriate gap

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Checkboxes: hover, focus, active, checked states
2 participants