Experience domain field in task creation as select component #3233
Conversation
| })( | ||
| <SelectExperienceDomain | ||
| disabled={isEditingMode} | ||
| mode="default" |
There was a problem hiding this comment.
Can you omit this line? If mode is undefined, antd should default automatically to "default", right?
| <SelectExperienceDomain | ||
| disabled={isEditingMode} | ||
| mode="default" | ||
| placeholder="Select a Experience Domain" |
| placeholder="Select a Experience Domain" | ||
| notFoundContent={messages["task.domain_does_not_exist"]} | ||
| width={100} | ||
| alreadyUsedDomains={[]} |
There was a problem hiding this comment.
Can't this be the default value so it doesn't need to be passed?
| alreadyUsedDomains={[]} | ||
| onSelect={null} | ||
| onChange={null} | ||
| value={null} |
There was a problem hiding this comment.
These three lines shouldn't be necessary ideally. Can you omit them? If flow complains, you probably need to add a default value via static defaultProps = { ... } (see here: https://reactjs.org/docs/typechecking-with-proptypes.html#default-prop-values).
| @@ -203,6 +204,10 @@ class ExperienceModalView extends React.PureComponent<Props, State> { | |||
| }; | |||
|
|
|||
| addEnteredExperience = (domain: string) => { | |||
| if (domain.length < 3) { | |||
| Toast.warning("A experience domain needs at least 3 letters."); | |||
| value={[]} | ||
| width={50} | ||
| onSelect={this.addEnteredExperience} | ||
| onDeselect={() => {}} | ||
| onChange={null} |
| @@ -35,18 +41,28 @@ class SelectExperienceDomain extends React.PureComponent<Props, State> { | |||
| } | |||
|
|
|||
| render() { | |||
| const options = this.getUnusedDomains().map(domain => <Option key={domain}>{domain}</Option>); | |||
| const notFoundContent = this.props.notFoundContent || "Not Found"; | |||
There was a problem hiding this comment.
I think this variable is not necessary. If it's undefined, antd will default to "Not found" automatically.
| @@ -35,18 +41,28 @@ class SelectExperienceDomain extends React.PureComponent<Props, State> { | |||
| } | |||
|
|
|||
| render() { | |||
| const options = this.getUnusedDomains().map(domain => <Option key={domain}>{domain}</Option>); | |||
| const notFoundContent = this.props.notFoundContent || "Not Found"; | |||
| const additionalProps = {}; | |||
There was a problem hiding this comment.
Since additionalProps is only used for one component (in contrast to an earlier state, where you used it for two different branches with JSX), it doesn't add much value. You can directly inline it into the JSX below like that:
<Select
...
onChange: this.props.onChange
It doesn't matter whether this.props.onChange is undefined or not (so no need for the if-switches).
…/webknossos into experience-field-as-select
…erience-field-as-select
| onSelect: () => {}, | ||
| value: null, | ||
| notFoundContent: null, | ||
| disabled: false, |
There was a problem hiding this comment.
Sorry to bug you again about that, but I think that alreadyUsedDomains is the only prop which needs to get a default here. All other props should be defined as nullable in the props type (e.g., mode?: string) which is why they don't need a default value. Does that work?
|
@philippotto Ok I applied your requested changes and they make sense to me :) |
|
Don't forget to update the changelog, though ;) |
URL of deployed dev instance (used for testing):
Steps to test:
the "change user experience"-modal should work as before
includes a bug fix, that disables entering domains with less than 3 letters in the modal
open the task creation
without selecting a exp domain and submitting the form should show an typical FieldDecorator error
selecting on should work and remove the waring from the FieldDecorator
create a task
check in the tasks view whether the exp domain is correct
Issues: