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
WB-483: Add custom labels to MultiSelect #659
Conversation
…nd selectItemType)
*/ | ||
selectItemType: 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'm deprecating these two props in favor of labels
.
@@ -184,6 +225,8 @@ export default class MultiSelect extends React.Component<Props, State> { | |||
searchText: "", | |||
lastSelectedValues: [], | |||
}; | |||
// merge custom labels with the default ones | |||
this.labels = {...defaultLabels, ...props.labels}; |
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 ended up creating this labels
instance variable to merge the custom labels with the default labels defined in constants.js
.
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.
Will this update if the props change later on?
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.
mmm It won't. I'll make the pertinent changes.
Codecov Report
@@ Coverage Diff @@
## master #659 +/- ##
==========================================
+ Coverage 95.48% 95.64% +0.16%
==========================================
Files 135 134 -1
Lines 2260 2275 +15
Branches 425 428 +3
==========================================
+ Hits 2158 2176 +18
+ Misses 96 93 -3
Partials 6 6
Continue to review full report at Codecov.
|
@@ -34,3 +34,15 @@ export const SEPARATOR_ITEM_HEIGHT = 9; | |||
|
|||
export const SEARCH_ITEM_HEIGHT = | |||
DROPDOWN_ITEM_HEIGHT + searchInputStyle.margin + searchInputStyle.marginTop; | |||
|
|||
// The default labels that will be used by different components | |||
export const defaultLabels = { |
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.
Now all the default labels are centralized in this file.
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.
Very nice!
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.
Excellent, thank you! A few questions inline.
* When this is true, the menu text shows "All {selectItemType}" when no | ||
* item is selected. | ||
* When this is true, the menu text shows "All items" when no item is | ||
* selected. |
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.
Might be worth mentioning which label this is, as well
@@ -184,6 +225,8 @@ export default class MultiSelect extends React.Component<Props, State> { | |||
searchText: "", | |||
lastSelectedValues: [], | |||
}; | |||
// merge custom labels with the default ones | |||
this.labels = {...defaultLabels, ...props.labels}; |
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.
Will this update if the props change later on?
clearSearch: defaultLabels.clearSearch, | ||
filter: defaultLabels.filter, | ||
}, | ||
}; |
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 notice you're not doing this this.labels
here, any reason?
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.
Yes, this component is only accessed by MultiSelect
and we are doing the corresponding this.labels
check in that component. In this case, we have two options:
- No custom labels set: It will use the values from
defaultValues
. - Custom labels set: It will override the values of
this.props.labels
with the values passed fromMultiSelect
.
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.
Disregard my previous comment. We still need to sync the labels
sent by the parent component, so I might to reuse the same logic to get the current labels
from MultiSelect
.
@@ -34,3 +34,15 @@ export const SEPARATOR_ITEM_HEIGHT = 9; | |||
|
|||
export const SEARCH_ITEM_HEIGHT = | |||
DROPDOWN_ITEM_HEIGHT + searchInputStyle.margin + searchInputStyle.marginTop; | |||
|
|||
// The default labels that will be used by different components | |||
export const defaultLabels = { |
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.
Very nice!
Summary
MultiSelect
Added the
labels
prop to theMultiSelect
component that allows us to pass a set of custom labels to the component.This is the set of keys allowed by the component:
Test plan
yarn test
.cc @annr