-
Notifications
You must be signed in to change notification settings - Fork 31
feat: add attrs to combobox #3169
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
base: main
Are you sure you want to change the base?
Conversation
View your CI Pipeline Execution ↗ for commit 5adaf71
☁️ Nx Cloud last updated this comment at |
View your CI Pipeline Execution ↗ for commit c7d4920
☁️ Nx Cloud last updated this comment at |
- Add OptionStrict type export to main gamut package index - Enables importing OptionStrict interface for select option typing - Snapshot dated: 2025-09-16 14:01:15
- Add ExtendedOption and SelectDropdownGroup type exports - Bump version to 66.4.2 to ensure changes are picked up - Enables importing these types for select dropdown typing
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.
Left some (super) nits and some Qs around typing. <- after some clarification I'd be happy to approve :)
The combobox attrs look great!
Asked via slack about the hidden <input>
element being duplicated, but if there's no workaround then 🤷
The color for Groups also look good :)
VO works as expected, i.e. compared to prod
Thanks for also fixing the act()
warnings!
}, | ||
}), | ||
option: (provided, state: OptionState) => { | ||
return { |
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.
also a super super nit: could opt to remove this return
and wrap the whole object in parens instead like the other styling?
|
||
### Abbreviated options | ||
|
||
⚠️ |
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.
Is this intentional?
disabled?: boolean; | ||
/** Optional text displayed on the right side of the option */ | ||
rightLabel?: string; | ||
/** Size specification for the option (typically 'small' or 'medium') */ |
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.
Q: is there a reason for the wording here, "typically 'small' or 'medium'"?
looks like those should be the only two sizes, could this be more strictly typed? Is there some kind of edge case?
/** Optional text displayed on the right side of the option */ | ||
rightLabel?: string; | ||
/** Size specification for the option (typically 'small' or 'medium') */ | ||
size?: string; |
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.
or maybe taken from SelectDropdownSizes
in component-props.ts?
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.
Or actually the BaseSelectProps
from packages/gamut/src/Form/SelectDropdown/types/styles.ts
?
*/ | ||
export interface SelectDropdownSizes { | ||
/** Visual size of the component */ | ||
size?: 'small' | 'medium'; |
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.
should this pull from packages/gamut/src/Form/SelectDropdown/types/styles.ts
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.
oop bad robot, i'll fix!!
📬Published Alpha Packages:@codecademy/[email protected] |
🚀 Styleguide deploy preview ready! |
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.
Looks great! the types look much DRYer!
* This is only relevant when the dropdown width is set to be larger or smaller than the input width. | ||
*/ | ||
menuAlignment?: 'left' | 'right'; | ||
/** Visual size of the component */ |
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.
leftover comment about size
Overview
Added ability to add data- + aria- attrs to combobox and swaps groupHeading color to text-secondary
I also broke some files down into smaller nested files + some act test warnings
PR Checklist
Testing Instructions
PR Links and Envs