-
Notifications
You must be signed in to change notification settings - Fork 2.6k
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: record group chevron button #9645
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.
PR Summary
Added a new AnimatedLightIconButton component that extends LightIconButton with Framer Motion animations, primarily used for record group chevron buttons.
- Added
packages/twenty-ui/src/input/button/components/AnimatedLightIconButton.tsx
with animation support via framer-motion ^11.18.0 - Duplicate
title
prop in AnimatedLightIconButtonProps (both explicit and in ComponentProps<'button'>) - Missing default case in accent color switch statement could lead to undefined colors
- Removed outline on focus in AnimatedLightIconButton may impact accessibility
- Replaced custom motion.span with AnimatedLightIconButton in RecordTableRecordGroupSection while maintaining animation behavior
6 file(s) reviewed, 3 comment(s)
Edit PR Review Bot Settings | Greptile
export type AnimatedLightIconButtonProps = { | ||
className?: string; | ||
testId?: string; | ||
Icon?: IconComponent; | ||
title?: string; | ||
size?: LightIconButtonSize; | ||
accent?: LightIconButtonAccent; | ||
active?: boolean; | ||
disabled?: boolean; | ||
focus?: boolean; | ||
onClick?: (event: MouseEvent<HTMLButtonElement>) => void; | ||
} & Pick<ComponentProps<'button'>, 'aria-label' | 'title'> & | ||
Pick<MotionProps, 'animate' | 'transition'>; |
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.
logic: title prop is duplicated - it appears in both the explicit props and ComponentProps<'button'> Pick
<AnimatedLightIconButton | ||
Icon={IconChevronDown} | ||
size="small" | ||
accent="secondary" | ||
animate={{ rotate: !isRecordGroupTableSectionToggled ? -90 : 0 }} | ||
transition={{ duration: theme.animation.duration.normal }} | ||
style={{ | ||
display: 'inline-block', | ||
}} | ||
> | ||
<IconChevronDown size={theme.icon.size.md} /> | ||
</motion.span> | ||
/> |
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.
logic: missing aria-label for accessibility on the button that controls expanding/collapsing the section
<StyledTrContainer onClick={handleDropdownToggle}> | ||
<td aria-hidden /> |
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.
style: consider adding role='row' to StyledTrContainer for better accessibility
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.
LGTM
This ticket is related to this Discord post https://discord.com/channels/1130383047699738754/1328756649657110559