Skip to content
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.

Sign up for GitHub

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

Jump to bottom

chore: fix helpers #602

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

chore: fix helpers #602

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
24 changes: 14 additions & 10 deletions packages/conform-react/helpers.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,6 @@ type InputProps = Pretty<
step?: string | number;
pattern?: string;
multiple?: boolean;
value?: string;
defaultChecked?: boolean;
defaultValue?: string;
}
Expand Down Expand Up @@ -258,13 +257,16 @@ export function getInputProps<Schema, Options extends InputOptions>(

if (typeof options.value === 'undefined' || options.value) {
if (options.type === 'checkbox' || options.type === 'radio') {
props.value = typeof options.value === 'string' ? options.value : 'on';
props.defaultValue =
typeof options.value === 'string' ? options.value : 'on';
props.defaultChecked =
typeof metadata.initialValue === 'boolean'
? metadata.initialValue
: metadata.initialValue === props.value;
: metadata.initialValue === props.defaultValue;
} else if (typeof metadata.initialValue === 'string') {
props.defaultValue = metadata.initialValue;
} else if (typeof metadata.initialValue === 'number') {
props.defaultValue = metadata.initialValue.toString();
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@edmundhung one thing that I was questioning, is should this even be initialValue? it seems value already does the casting for us via serialize

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think serialize should cast everything to string already. So this is probably not needed.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

and also from the #613 PR, we should probably also serialize bigint here too unless we use metadata.value

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The initialValue you got from the metadata is already serialized. So once #613 is merged, this should be good.

}
}

Expand Down Expand Up @@ -383,20 +385,22 @@ export function getCollectionProps<
metadata: FieldMetadata<Schema, any, any>,
options: Options,
): Array<
InputProps & Pick<Options, 'type'> & Pick<Required<InputProps>, 'value'>
InputProps &
Pick<Options, 'type'> &
Pick<Required<InputProps>, 'defaultValue'>
> {
return options.options.map((value) =>
return options.options.map((defaultValue) =>
simplify({
...getFormControlProps(metadata, options),
key: `${metadata.key ?? ''}${value}`,
id: `${metadata.id}-${value}`,
key: `${metadata.key ?? ''}${defaultValue}`,
id: `${metadata.id}-${defaultValue}`,
type: options.type,
value,
defaultValue,
defaultChecked:
typeof options.value === 'undefined' || options.value
? options.type === 'checkbox' && Array.isArray(metadata.initialValue)
? metadata.initialValue.includes(value)
: metadata.initialValue === value
? metadata.initialValue.includes(defaultValue)
: metadata.initialValue === defaultValue
: undefined,

// The required attribute doesn't make sense for checkbox group
Expand Down
8 changes: 4 additions & 4 deletions playground/app/routes/collection.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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">
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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>
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is definitely a breaking change now unless we preserve props.value 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@edmundhung yeah, maybe we can have a call on discord and do some brain storming if you're available

</label>
))}
</Field>
Expand All @@ -56,9 +56,9 @@ export default function Example() {
type: 'checkbox',
options: ['a', 'b', 'c', 'd'],
}).map((props) => (
<label key={props.value} className="inline-block">
<label key={props.defaultValue} className="inline-block">
<input {...props} />
<span className="p-2">{props.value?.toUpperCase()}</span>
<span className="p-2">{props.defaultValue.toUpperCase()}</span>
</label>
))}
</Field>
Expand Down
2 changes: 1 addition & 1 deletion playground/app/routes/dom-value.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,7 @@ export default function Example() {
}).map((props) => (
<label key={props.key}>
<input {...props} />
{props.value}
{props.defaultValue}
</label>
))}
</Field>
Expand Down
12 changes: 6 additions & 6 deletions playground/app/routes/input-attributes.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -189,12 +189,12 @@ export default function Example() {
? fields.released.descriptionId
: undefined,
}).map((props) => (
<label key={props.value} className="inline-block">
<label key={props.defaultValue} className="inline-block">
<input {...props} />
<span className="p-2">
{`${props.value?.slice(0, 1).toUpperCase()}${props.value
?.slice(1)
.toLowerCase()}`}
{`${props.defaultValue
.slice(0, 1)
.toUpperCase()}${props.defaultValue.slice(1).toLowerCase()}`}
</span>
</label>
))}
Expand All @@ -207,9 +207,9 @@ export default function Example() {
? fields.languages.descriptionId
: undefined,
}).map((props) => (
<label key={props.value} className="inline-block">
<label key={props.defaultValue} className="inline-block">
<input {...props} />
<span className="p-2">{props.value?.toUpperCase()}</span>
<span className="p-2">{props.defaultValue.toUpperCase()}</span>
</label>
))}
</Field>
Expand Down
16 changes: 8 additions & 8 deletions tests/conform-react.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -117,17 +117,17 @@ describe('conform-react', () => {
});
expect(getInputProps(metadata, { type: 'checkbox' })).toEqual({
...props,
type: 'checkbox',
value: 'on',
defaultChecked: false,
defaultValue: 'on',
type: 'checkbox',
});
expect(
getInputProps({ ...metadata, initialValue: 'on' }, { type: 'radio' }),
).toEqual({
...props,
type: 'radio',
value: 'on',
defaultChecked: true,
defaultValue: 'on',
type: 'radio',
});
expect(
getInputProps(
Expand All @@ -136,9 +136,9 @@ describe('conform-react', () => {
),
).toEqual({
...props,
type: 'checkbox',
value: 'something else',
defaultChecked: true,
defaultValue: 'something else',
type: 'checkbox',
});
expect(
getInputProps(
Expand All @@ -147,9 +147,9 @@ describe('conform-react', () => {
),
).toEqual({
...props,
type: 'checkbox',
value: 'something else',
defaultChecked: true,
defaultValue: 'something else',
type: 'checkbox',
});
expect(getInputProps(metadata, { type: 'file' })).toEqual({
...props,
Expand Down