-
Notifications
You must be signed in to change notification settings - Fork 30
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(Disclosure): creating an Disclosure component #2859
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.
looking great!! left a couple comments for semantic type things but this is looking really nice
packages/styleguide/stories/Molecules/Disclosure/index.stories.mdx
Outdated
Show resolved
Hide resolved
Going to leave this PR in limbo for the time being since we need to work on refactoring list/expandedrow to accommodate Disclosure. However, all styling and feedback is addressed and should be reviewed. |
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.
this is looking great! only point of feedback is it looks like the Disclosure animates on open but "snaps" shut. i remember having some similar issues when developing the ExpandableList row so maybe you can steal some styles from there. nice work!!
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.
wonderful!! sounds great on VO and looks great too. holding on ✅ for the animations only
expanded: { height: 'auto' }, | ||
collapsed: { height: 0 }, | ||
}} | ||
transition={{ duration: 0.5, ease: 'easeInOut' }} |
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.
this seems a little slow - you could double check with stacey but i think we'd want it to match List
(0.2, or timingValues.medium / 1000
if you wanna be fancy!)
@@ -0,0 +1,23 @@ | |||
import { motion } from 'framer-motion'; | |||
|
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.
love this!!! starting to think we need more gamut documentation on our animations (not in this PR but maybe another ticket for the backlog ✨ )
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.
Ah, you're right -- I'll cut a ticket!
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.
🔥 🔥 🔥
📬Published Alpha Packages:@codecademy/gamut@55.22.7-alpha.c7b611.0 |
🚀 Styleguide deploy preview ready! |
Overview
Adding a Disclosure component to replace the current AccordionDeprecated component.
PR Checklist
Testing Instructions
path=/docs/molecules-disclosure--disclosure
(Mono)
admin/portal
(Monolith)
Don't make me tap the sign.
PR Links and Envs