-
-
Notifications
You must be signed in to change notification settings - Fork 32.4k
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
[Autocomplete] Add new AutocompleteInputChangeReason #37301
[Autocomplete] Add new AutocompleteInputChangeReason #37301
Conversation
Netlify deploy previewBundle size reportDetails of bundle changes (Toolpad) |
Hi! isn't this PR missing things from the original one (#37013)? for example this change. |
9bbfb41
to
82e36d9
Compare
I added the missing changes that were present in #37013 |
@michaldudak @DiegoAndai this is ready for review. |
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.
@aarongarciah we should add the following
- A section in the v6 migration guide explaining the change and how to adapt
- Tests for the introduced reasons
Do you agree? Are we missing anything else?
@DiegoAndai that should cover everything. I'll add it. |
b5b2ba8
to
41351fa
Compare
41351fa
to
c33f43f
Compare
- `"blur"`: like `"reset"` but triggered when the focus is moved off the input. `clearOnBlur` must be `true`. Before, it was treated as `"reset"`. | ||
- `"selectOption"`: triggered when the input value changes after an option has been selected. Before, it was treated as `"reset"`. | ||
- `"removeOption"`: triggered in multiple selection when a chip gets removed due to the corresponding option being selected. Before, it was treated as `"reset"`. |
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.
@michaldudak let me know if these descriptions are accurate.
@DiegoAndai just pushed the tests covering the missing |
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.
Nice @aarongarciah! One final question. Do you think we should take this opportunity to use user event instead of fireEvent
on these tests? If you prefer doing it on a separate PR, that's ok. Let me know.
docs/data/material/migration/migrating-to-v6/migrating-to-v6.md
Outdated
Show resolved
Hide resolved
9a5b265
to
5adc07f
Compare
@DiegoAndai all tests updated to use the new |
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.
Great addition ⭐
Tests getting refactored ✨
Amazing job 💯
Co-authored-by: Aarón García Hervás <[email protected]>
fixes: #36085
duplicate of #37013