-
Notifications
You must be signed in to change notification settings - Fork 2
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
fix(select): Create a generic Id type for Option interface #214
Changes from 1 commit
50386f1
c622ed9
cb18789
a07ff68
80df9c7
ef83062
18781d5
a8e83a8
d0fce58
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||||
---|---|---|---|---|---|---|---|---|
@@ -1,13 +1,19 @@ | ||||||||
import React from "react"; | ||||||||
|
||||||||
interface Option { | ||||||||
id: string; | ||||||||
interface Option<Id = string> { | ||||||||
id: Id; | ||||||||
title: React.ReactNode; | ||||||||
isDisabled?: boolean; | ||||||||
} | ||||||||
|
||||||||
interface TypeaheadSelectOption extends Option { | ||||||||
title: string; | ||||||||
} | ||||||||
// TypeaheadSelectOption is intentionally empty. It happens not | ||||||||
// to have more properties than Option, but this may | ||||||||
// change in the future, and it helps to have a TypeaheadSelectOption | ||||||||
// interface that people can use. Therefore the no-empty-interface is disabled | ||||||||
// rule for this declaration: | ||||||||
|
||||||||
// eslint-disable-next-line @typescript-eslint/no-empty-interface | ||||||||
interface TypeaheadSelectOption extends Option {} | ||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We can use type instead of an interface to avoid disabling this rule.
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I tried the way you said, but it didn't let me set a generic type to |
||||||||
|
||||||||
type SelectItemElement = HTMLLIElement | HTMLDivElement; | ||||||||
|
||||||||
|
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.
Why did we add
title
property here?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 think it can be useful for some cases, but I can set it as optional or remove it completely.