-
Notifications
You must be signed in to change notification settings - Fork 2.2k
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
Use generics for Select arrays #7036
Changes from 2 commits
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 |
---|---|---|
|
@@ -40,13 +40,13 @@ const INTENTS = [Intent.NONE, Intent.PRIMARY, Intent.SUCCESS, Intent.DANGER, Int | |
|
||
export interface MultiSelectExampleState { | ||
allowCreate: boolean; | ||
createdItems: Film[]; | ||
createdItems: readonly Film[]; | ||
disabled: boolean; | ||
fill: boolean; | ||
films: Film[]; | ||
films: readonly Film[]; | ||
hasInitialContent: boolean; | ||
intent: boolean; | ||
items: Film[]; | ||
items: readonly Film[]; | ||
matchTargetWidth: boolean; | ||
openOnKeyDown: boolean; | ||
popoverMinimal: boolean; | ||
|
@@ -124,7 +124,7 @@ export class MultiSelectExample extends React.PureComponent<ExampleProps, MultiS | |
|
||
return ( | ||
<Example options={this.renderOptions()} {...this.props}> | ||
<MultiSelect<Film> | ||
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. The omission of the specified
Another way to solve this is to explicitly define the second type parameter, e.g. <MultiSelect<Film, readonly Film[]> |
||
<MultiSelect | ||
{...flags} | ||
createNewItemFromQuery={allowCreate ? createFilms : undefined} | ||
createNewItemRenderer={allowCreate ? renderCreateFilmsMenuItem : null} | ||
|
@@ -251,7 +251,9 @@ export class MultiSelectExample extends React.PureComponent<ExampleProps, MultiS | |
); | ||
} | ||
|
||
private renderCustomTarget = (selectedItems: Film[]) => <MultiSelectCustomTarget count={selectedItems.length} />; | ||
private renderCustomTarget = (selectedItems: readonly Film[]) => ( | ||
<MultiSelectCustomTarget count={selectedItems.length} /> | ||
); | ||
|
||
private renderTag = (film: Film) => film.title; | ||
|
||
|
@@ -287,16 +289,16 @@ export class MultiSelectExample extends React.PureComponent<ExampleProps, MultiS | |
this.selectFilms([film]); | ||
} | ||
|
||
private selectFilms(filmsToSelect: Film[]) { | ||
private selectFilms(filmsToSelect: readonly Film[]) { | ||
this.setState(({ createdItems, films, items }) => { | ||
let nextCreatedItems = createdItems.slice(); | ||
let nextFilms = films.slice(); | ||
let nextItems = items.slice(); | ||
|
||
filmsToSelect.forEach(film => { | ||
const results = maybeAddCreatedFilmToArrays(nextItems, nextCreatedItems, film); | ||
nextItems = results.items; | ||
nextCreatedItems = results.createdItems; | ||
nextItems = results.items.slice(); | ||
nextCreatedItems = results.createdItems.slice(); | ||
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. Do we have to slice these? We're not actually mutating either of them below. 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. Ah, yeah, good call. I originally added the slice here since, otherwise, we'd be assigning the readonly arrays from results ( |
||
// Avoid re-creating an item that is already selected (the "Create | ||
// Item" option will be shown even if it matches an already selected | ||
// item). | ||
|
@@ -336,7 +338,7 @@ export class MultiSelectExample extends React.PureComponent<ExampleProps, MultiS | |
} | ||
}; | ||
|
||
private handleFilmsPaste = (films: Film[]) => { | ||
private handleFilmsPaste = (films: readonly Film[]) => { | ||
// On paste, don't bother with deselecting already selected values, just | ||
// add the new ones. | ||
this.selectFilms(films); | ||
|
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.
The modification of the examples here in this PR isn't strictly necessary, though it helps to serve as a practical example of how to migrate existing Select/MultiSelect components to use readonly arrays. It's also worth noting that the examples here still compile fine if they are not switched to use readonly arrays, demonstrating the backwards compatibility of these type changes.
All the docs/test changes are contained within: 19cdbee