-
Notifications
You must be signed in to change notification settings - Fork 367
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
refactor: [M3-6919] - replace remaining react-select instances & types in Linodes Feature #11509
base: develop
Are you sure you want to change the base?
Conversation
onChange={(e, value) => handleChartRangeChange(value)} | ||
options={options} | ||
sx={{ mt: 1, width: 150 }} |
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.
nothing changed here, just linting - only change is above
@@ -506,19 +506,18 @@ export const IPSharingRow: React.FC<SharingRowProps> = React.memo((props) => { | |||
</Grid> | |||
<Grid sm={10} xs={12}> | |||
<StyledSelect | |||
onChange={(_, selected: AutocompleteOption<string>) => |
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.
I really like how Autocomplete allows you to pass any type of options
and the onChange
types get properly inferred. This doesn't seem to be the case with Select
😥
Select | Autocomplete |
---|---|
Does it make sense to / can we improve the types on Select
? This seems like a step backwards in DX compared to Autocomplete because we have to manually type this.
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.
No need for sadness! Thanks for suggesting this improvement.
I added inference for the onChange
which is indeed a nice improvement indeed.
As far as the ability to add any type of option, I'd like to keep things the way they are for now, and see if this is something we'd really want to change in the future (would add quite a bit of complexity). Despite being build on top of the Autocomplete
, this select aims to limit the amount of things we can do with it (for more "simple" use and without complicating the Autocomplete further - they both serve their different purpose and I'd like to keep it that way we can help it).
If the need arises we can revisit!
a296cb9
to
71ee59e
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.
IP Sharing & Disk select behavior ✅
Code review ✅
Can we get a changeset added?
71ee59e
to
bd0169b
Compare
Coverage Report: ✅ |
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.
✅ disk select
✅ ip sharing
thank you!
Cloud Manager UI test results🎉 474 passing tests on test run #9 ↗︎
|
Description 📝
Continuing the replacement of react-select through the app. This one targets left over instances & borrowed types in Linode detail
Changes 🔄
DiskSelect
componentItem
Preview 📷
No visual or functional regression to expect from this PR
How to test 🧪
Verification steps
/linodes/{id}/networking
> IP Sharing/linodes/{id}/settings
Author Checklists
As an Author, to speed up the review process, I considered 🤔
👀 Doing a self review
❔ Our contribution guidelines
🤏 Splitting feature into small PRs
➕ Adding a changeset
🧪 Providing/improving test coverage
🔐 Removing all sensitive information from the code and PR description
🚩 Using a feature flag to protect the release
👣 Providing comprehensive reproduction steps
📑 Providing or updating our documentation
🕛 Scheduling a pair reviewing session
📱 Providing mobile support
♿ Providing accessibility support
As an Author, before moving this PR from Draft to Open, I confirmed ✅