-
Notifications
You must be signed in to change notification settings - Fork 4
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
fix(Select): expose generic types to allow by to work #2008
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## next #2008 +/- ##
=======================================
Coverage 97.58% 97.58%
=======================================
Files 109 109
Lines 2572 2572
Branches 645 645
=======================================
Hits 2510 2510
Misses 60 60
Partials 2 2 ☔ View full report in Codecov by Sentry. |
size-limit report 📦
|
I need to do some validation checks on this to make sure |
string | { [k: string]: unknown }, | ||
{ [k: string]: unknown } |
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 first line represents what is allowed in things like defaultValue
or value
the second line represents what gets used when by
is evaluated
- when a simple string,
by
is string - when an object,
by
is the type of the keys on that object (usuallystring | number
), or that eval function - ...
9a96f9a
to
4a0343f
Compare
- borrow type value determination from HeadlessUI internals - add test to make sure types used map to params
4a0343f
to
baf76da
Compare
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. Thanks @booc0mtaco!
*/ | ||
export const WithSelectedBy: StoryObj<typeof Select> = { | ||
args: { | ||
...WithSelectedOption.args, |
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.
[non-blocking] It might be a more thorough test to use defaultValue: {...exampleOptions[1]}
instead of inheriting defaultValue: exampleOptions[1]
from WithSelectedOption
since the purpose of by
is to handle cases where the values are copies rather than the exact same objects.
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.
++ good call. I suspect it will work properly, but this will prevent some by-ref getting thru down the road
## [15.1.0](v15.0.1...v15.1.0) (2024-07-15) [Storybook](https://61313967cde49b003ae2a860-qztphlqyid.chromatic.com/) ### Features * add runtime warning/errors to components ([#2007](#2007)) ([661130b](661130b)) * **InputField:** add show/hide button for password field type ([#2006](#2006)) ([52d9ca0](52d9ca0)) * **Modal:** add height property to modal API ([#2011](#2011)) ([8d0c68f](8d0c68f)) ### Bug Fixes * **Icon:** update pencil icon to latest design ([#2016](#2016)) ([cb8d1a7](cb8d1a7)) * **Link:** apply font weight to standalone sizes ([#2015](#2015)) ([2e47271](2e47271)) * **Select:** expose generic types to allow by to pass type checks ([#2008](#2008)) ([421c91b](421c91b))
Test Plan: