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.

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

Add a default unique id for useInput #9788

Merged
merged 16 commits into from May 2, 2024

Conversation

erwanMarmelab
Copy link
Contributor

@erwanMarmelab erwanMarmelab commented Apr 22, 2024

To do

  • change default id
  • adapt tests
  • Add a section in the upgrade guide

Closes #9534

@@ -132,7 +132,7 @@ export const useInput = <ValueType = any>(
};

return {
id: id || finalSource,
id: id || `use-input-${finalSource}`,
Copy link
Contributor

Choose a reason for hiding this comment

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

We should leverage useId if id is not provided

Copy link
Contributor Author

Choose a reason for hiding this comment

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

applied

id: id || finalSource,
id:
id ||
`${defaultId.substring(1, defaultId.length - 1)}-${finalSource}`,
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not just defaultId?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It was to be more readable.
Applied

docs/useInput.md Outdated
| `format` | Optional | `Function` | - | A function to format the value from the record to the input value |
| `parse` | Optional | `Function` | - | A function to parse the value from the input to the record value |
| `validate` | Optional | `Function` &#124; `Function[]` | - | A function or an array of functions to validate the input value |
| `id` | Optional | `string` | `:r[input number]:` | The id of the input |
Copy link
Member

Choose a reason for hiding this comment

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

too much details here. Just use autogenerated for the default 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.

done

id: id || finalSource,
id:
id ||
`${defaultId.substring(1, defaultId.length - 1)}-${finalSource}`,
Copy link
Member

Choose a reason for hiding this comment

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

I don't understand why you substring

Copy link
Contributor Author

Choose a reason for hiding this comment

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

removed

@fzaninotto
Copy link
Member

You should probably add a section in the upgrade guide as this is a breaking change

Comment on lines +931 to +936
## Inputs default ids are auto-generated

In previous versions, the input default id was the source of the input. In v5, inputs defaults ids are auto-generated with [React useId()](https://react.dev/reference/react/useId).

**Tip:** You still can pass an id as prop of any [react-admin input](./Inputs.md) or use a [reference](https://fr.react.dev/reference/react/useRef).

Copy link
Contributor

Choose a reason for hiding this comment

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

What's the impact for users? You should mention that if they relied on this for tests, they should pass the id

Copy link
Contributor Author

Choose a reason for hiding this comment

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

something like this ?

@djhi djhi added this to the 5.0.0 milestone May 2, 2024
@djhi djhi merged commit 3239c64 into next May 2, 2024
12 checks passed
@djhi djhi deleted the feat/add-default-unique-id-for-useInput branch May 2, 2024 15:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
RFR Ready For Review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants