chore: fix helpers #602
chore: fix helpers #602
Conversation
lifeiscontent
force-pushed
the
chore/fix-helpers
branch
4 times, most recently
from
a251d54 to
a32fca5
Compare
1. Fix `getInputProps` to return a `defaultValue` if the `defaultValue` is a number 2. Fix `getInputProps` to return `defaultValue` instead of `value` for `checkbox` and `radio` input types 3. Fix `getCollectionProps` to return `defaultValue` instead of `value`
lifeiscontent
force-pushed
the
chore/fix-helpers
branch
from
a32fca5 to
30dc0a6
Compare
| } else if (typeof metadata.initialValue === 'string') { | ||
| props.defaultValue = metadata.initialValue; | ||
| } else if (typeof metadata.initialValue === 'number') { | ||
| props.defaultValue = metadata.initialValue.toString(); |
There was a problem hiding this comment.
@edmundhung one thing that I was questioning, is should this even be initialValue? it seems value already does the casting for us via serialize
There was a problem hiding this comment.
I think serialize should cast everything to string already. So this is probably not needed.
There was a problem hiding this comment.
if we use initialValue its not a string, its the the fully represented type I have patched this functionality locally because previously it was returning nothing for values that were numbers.
There was a problem hiding this comment.
and also from the #613 PR, we should probably also serialize bigint here too unless we use metadata.value
There was a problem hiding this comment.
The initialValue you got from the metadata is already serialized. So once #613 is merged, this should be good.
| @@ -45,9 +45,9 @@ export default function Example() { | |||
| type: 'radio', | |||
| options: ['x', 'y', 'z'], | |||
| }).map((props) => ( | |||
| <label key={props.value} className="inline-block"> | |||
| <label key={props.defaultValue} className="inline-block"> | |||
There was a problem hiding this comment.
This looks like a breaking change to me at first 😅 But it seems like the example in the docs use id. So it should be fine..
| <input {...props} /> | ||
| <span className="p-2">{props.value?.toUpperCase()}</span> | ||
| <span className="p-2">{props.defaultValue.toUpperCase()}</span> |
There was a problem hiding this comment.
This is definitely a breaking change now unless we preserve props.value 🤔
There was a problem hiding this comment.
to be honest, I have some ideas on how to improve the collection helper, e.g. sometimes you want the item rather than just the value so you can provide better labels or more info like a badge or something, but not sure if that's worth doing now.
There was a problem hiding this comment.
@edmundhung would you want to chat about making these helpers more flexible? I could also create a gist of some ideas if you don't have time to discuss at the moment. It might make sense to do it now if you're open to making a breaking change
There was a problem hiding this comment.
I would avoid a breaking change unless it has huge impact at this stage. At least not when v1 is just released 3 months ago 😅 But I would still be happy to learn more about your ideas even there is breaking changes. I am also considering getting rid of all these exports in v2 and provide a CLI that generate these helpers similar to shadcn-ui. Then we don't need to care about breaking changes on these helpers anymore.
There was a problem hiding this comment.
@edmundhung yeah, maybe we can have a call on discord and do some brain storming if you're available
getInputPropsto return a defaultValue if the defaultValue is a numbergetInputPropsto returndefaultValueinstead ofvalueforcheckboxandradioinput typesgetCollectionPropsto returndefaultValueinstead ofvalue