-
Notifications
You must be signed in to change notification settings - Fork 0
Lukebailey/infra 386 create a a11y checkbox #53
base: main
Are you sure you want to change the base?
Lukebailey/infra 386 create a a11y checkbox #53
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The new dependencies will need to be added to:
https://github.com/contra/ui-kit/blob/main/bin/build-cjs.js#L11
Ah thanks @erictaylor! I didn't realise this was a thing (I'm new to cjs), will add these in today. Also, as I'm the author of this PR I cannot approve and merge the changes requested, what is the best way to proceed here? |
…tion' into lukebailey/infra-386-create-a-a11y-checkbox
…rops and ToggleProps
…ailey/infra-387-explore-and-create-a-micro-interaction
…tion' into lukebailey/infra-386-create-a-a11y-checkbox
.gitignore
Outdated
*.log | ||
.vscode/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's not add .vscode to the ignore. We do like to share some configuration in projects. This project doesn't have anything shared yet, but I'd imagine we would do some similiar sharing that we do in https://github.com/contra/contra-web-app/tree/master/.vscode
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep! This has been fixed in 55ab973
package.json
Outdated
@@ -85,6 +86,8 @@ | |||
"react": "^16.14.0 || ^17.0.0" | |||
}, | |||
"dependencies": { | |||
"@stitches/react": "^1.2.5" | |||
"gsap": "^3.9.1", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have some slight concerns about bringing gsap in as a dependency. gsap is large library, and I'm curious how much we need it as opposed to handling things in a more light weight manner.
Would love to hear some thoughts on this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought this maybe a thing! totally fine to remove GSAP and go with CSS transitions for the UI kit. See comment #53 (comment)
src/animationIcons/CheckIcon.tsx
Outdated
const Path = styled('path', { | ||
'input:checked ~ div &': { | ||
transition: 'stroke-dashoffset 1s cubic-bezier(0.16, 1, 0.3, 1)', | ||
}, | ||
transition: 'stroke-dashoffset 0.2s cubic-bezier(0.11, 0, 0.5, 0)', | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should use variants to essentially add a isChecked
prop that would adjust the transition style.
The reason I would do this is that the style then relies on a prop and not the structure of the DOM.
Right now this styling is very conditional based on where it is used and isn't really sharable.
Thoughts @Boeing787 and @lukerohanbailey?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed! See comment #53 (comment)
src/components/Checkbox.tsx
Outdated
text, | ||
defaultSelected, | ||
isIndeterminate, | ||
...restProps |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I usually highly advise not using rest props on component libraries like this, and to specifically list all props out so you can make sure you are never spreading props improperly to base react components (if you did you'll get those annoying react warnings about a prop not being recognized).
In general it would be very easy for a consumer to pass a prop that could ultimately end up getting inappropriately passed all the way down to base component that it shouldnt.
@lukerohanbailey feel free to reach out to me for a Tuple if you'd like and we can walk through some changes together (if desired). I realize that getting all the context or answers to questions is difficult going back and forth in just comments. |
6186d32
to
5c060e9
Compare
@erictaylor I've made some changes that remove GSAP and replace it with CSS transitions that use the stitches variants feature e8cf1a9 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking great. Couple small things.
@@ -85,6 +86,7 @@ | |||
"react": "^16.14.0 || ^17.0.0" | |||
}, | |||
"dependencies": { | |||
"@stitches/react": "^1.2.5" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see we moved this from dependencies to devDependencies. If we are going to do this, then we need to add @stitches/react
to peerDependencies
which would constitute a major/breaking change.
import { easeOutExpo } from '../primitives/animation'; | ||
|
||
const Label = styled('label', { | ||
'& input:checked:disabled + div': { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What are we needing to target specifically with div
here? Can we tighten this to reference IconContainer specifically?
// Easing | ||
export const easeInQuad = 'cubic-bezier(0.55, 0.085, 0.68, 0.53)'; | ||
export const easeInCubic = 'cubic-bezier(0.55, 0.055, 0.675, 0.19)'; | ||
export const easeInQuart = 'cubic-bezier(0.895, 0.03, 0.685, 0.22)'; | ||
export const easeInQuint = 'cubic-bezier(0.755, 0.05, 0.855, 0.06)'; | ||
export const easeInExpo = 'cubic-bezier(0.95, 0.05, 0.795, 0.035)'; | ||
export const easeInCirc = 'cubic-bezier(0.6, 0.04, 0.98, 0.335)'; | ||
export const easeOutQuad = 'cubic-bezier(0.25, 0.46, 0.45, 0.94)'; | ||
export const easeOutCubic = 'cubic-bezier(0.215, 0.61, 0.355, 1)'; | ||
export const easeOutQuart = 'cubic-bezier(0.165, 0.84, 0.44, 1)'; | ||
export const easeOutQuint = 'cubic-bezier(0.23, 1, 0.32, 1)'; | ||
export const easeOutExpo = 'cubic-bezier(0.19, 1, 0.22, 1)'; | ||
export const easeOutCirc = 'cubic-bezier(0.075, 0.82, 0.165, 1)'; | ||
export const easeInOutQuad = 'cubic-bezier(0.455, 0.03, 0.515, 0.955)'; | ||
export const easeInOutCubic = 'cubic-bezier(0.645, 0.045, 0.355, 1)'; | ||
export const easeInOutQuart = 'cubic-bezier(0.77, 0, 0.175, 1)'; | ||
export const easeInOutQuint = 'cubic-bezier(0.86, 0, 0.07, 1)'; | ||
export const easeInOutExpo = 'cubic-bezier(1, 0, 0, 1)'; | ||
export const easeInOutCirc = 'cubic-bezier(0.785, 0.135, 0.15, 0.86)'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if it be better for us to export this an an object so you can import one item and have it strongly typed and autosuggested.
ie
export const animation = Object.freeze({
easeInQuad: 'cubic-bezier(0.55, 0.085, 0.68, 0.53),
// ...
}) as const;
Then consumption simply becomes
import { animation } from '@contra/ui-kit';
console.log(animation.easeInQuad);
Additionally, let's make sure we export this via ./src/index.ts
.
Consider naming animationTimingFunction
(long winded but accurate).
Summary
Create a a11y checkbox per the V3 Library on Figma, with states for:
References
Linear: https://linear.app/contra/issue/INFRA-386/create-a-a11y-checkbox
Loom: https://www.loom.com/share/7c8ef7dc185646bf9d6cdc60c25ed109
Figma: https://www.figma.com/file/owwo3mjL0dCKJijKGaf1XB/V3-Library?node-id=705%3A9117