-
Notifications
You must be signed in to change notification settings - Fork 2
Multi value text choices #1923
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
base: develop
Are you sure you want to change the base?
Multi value text choices #1923
Conversation
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.
Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.
| textChoiceValidator: undefined, | ||
| valueExpression: undefined, | ||
| textChoiceValidator: isTextChoice ? field.textChoiceValidator: undefined, | ||
| valueExpression: isTextChoice ? field.valueExpression: undefined, |
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.
nit: run the linter/formatter to clean these up.
| regexValidators: [], | ||
| scale: MAX_TEXT_LENGTH, | ||
| }) as DomainField; | ||
| if (!wasTextChoice) { |
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.
Couldn't we move this if statement logic into the if statement above so it looks something like:
if ((field.isTextChoiceField() || field.isMultiChoiceField()) && !wasTextChoice) {or maybe to be a little more verbose, but easier to read:
const isChoiceField = field.isTextChoiceField() || field.isMultiChoiceField();
if (isChoiceField && !wasTextChoice) {| {expanded && | ||
| !isFieldFullyLocked(field.lockType) && | ||
| !appPropertiesOnly && | ||
| DomainField.allowAdvancedSettings(field) && ( | ||
| <button | ||
| className="domain-row-button btn btn-default" | ||
| disabled={isFieldFullyLocked(field.lockType)} | ||
| id={createFormInputId(DOMAIN_FIELD_ADV, domainIndex, index)} | ||
| name={createFormInputName(DOMAIN_FIELD_ADV)} | ||
| onClick={this.onShowAdvanced} | ||
| type="button" | ||
| > | ||
| Advanced Settings | ||
| </button> | ||
| )} |
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.
If we move the logic for showing this button to a variable above:
const showAdvancedSettingsButton =
expanded &&
!isFieldFullyLocked(field.lockType) &&
!appPropertiesOnly &&
DomainField.allowAdvancedSettings(field);Then we can remove the added level of nesting in the TSX:
| {expanded && | |
| !isFieldFullyLocked(field.lockType) && | |
| !appPropertiesOnly && | |
| DomainField.allowAdvancedSettings(field) && ( | |
| <button | |
| className="domain-row-button btn btn-default" | |
| disabled={isFieldFullyLocked(field.lockType)} | |
| id={createFormInputId(DOMAIN_FIELD_ADV, domainIndex, index)} | |
| name={createFormInputName(DOMAIN_FIELD_ADV)} | |
| onClick={this.onShowAdvanced} | |
| type="button" | |
| > | |
| Advanced Settings | |
| </button> | |
| )} | |
| {showAdvancedSettingsButton && ( | |
| <button | |
| className="domain-row-button btn btn-default" | |
| disabled={isFieldFullyLocked(field.lockType)} | |
| id={createFormInputId(DOMAIN_FIELD_ADV, domainIndex, index)} | |
| name={createFormInputName(DOMAIN_FIELD_ADV)} | |
| onClick={this.onShowAdvanced} | |
| type="button" | |
| > | |
| Advanced Settings | |
| </button> | |
| )} |
It's a small change but I think it's more readable because the variable is named and there's less logic spread out over multiple lines in an already highly nested TSX block.
| if ( | ||
| (type === 'TextChoice' || type === 'MultiChoice') && | ||
| !rawPropertyValidator[i]?.properties?.validValues | ||
| ) { |
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 would define a variable isChoice below the hasExpressionStr variable above:
const isChoice = type === 'TextChoice' || type === 'MultiChoice';Then the if statement will fit on one line instead of 4, and be a little easier to read.
| if ( | |
| (type === 'TextChoice' || type === 'MultiChoice') && | |
| !rawPropertyValidator[i]?.properties?.validValues | |
| ) { | |
| if (isChoice && !rawPropertyValidator[i]?.properties?.validValues) { |
|
|
||
| async function convertRowToEditorModelData( | ||
| data: boolean | number | string, | ||
| data: [] | boolean | number | 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.
Could this type be more specific? Is it really an array of anything or is it more specific like an array of strings? Even if it's an array of primitive values we could do something like:
data: boolean | boolean[] | number | number[] | string | 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.
Good point. For now we only support string[]
| allowRelativeDateFilter={allowRelativeDateFilter} | ||
| disabled={hasNotInQueryFilter} | ||
| field={activeField} | ||
| fieldFilters={currentFieldFilters?.map(filter => filter.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.
This should be memoized above, otherwise it's going to produce a new array every render cycle, and force a rerender.
| } | ||
| key={activeFieldKey} | ||
| onFieldFilterUpdate={(newFilters, index) => | ||
| onFilterUpdate(activeField, newFilters, index) |
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.
This should be memoized above, via useCallback, otherwise it's going to produce a new method every render cycle, and force a rerender.
| <div className="field-modal__col-sub-title"> | ||
| Find values for {activeField.caption} | ||
| </div> | ||
| <FilterFacetedSelector |
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.
Like above, this <FilterFacetedSelector has several props being passed to it need to be memoized via useMemo or useCallback.
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.
Done
| options={filterOptions} | ||
| placeholder="Select a filter type..." | ||
| required={true} | ||
| value={fieldFilters?.[0]?.getFilterType()?.getURLSuffix() || 'arraycontainsall'} |
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.
Is this the correct use of ?. If fieldFilters[0] isn't null/undefined, then wouldn't it be guaranteed that getFilterType would have a non-null return? Basically, could we go from:
fieldFilters?.[0]?.getFilterType()?.getURLSuffix()
to
fieldFilters?.[0]?.getFilterType().getURLSuffix()
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.
There's an unused import of Fragment at the top of this file that should probably be cleaned up
Rationale
This PR enables "Multi Choice" data type in domain designer and supports editing/sorting multi values in the app.
Related Pull Requests
Changes
SelectInputto skipJoinValues to align with its actual usageFilterFacetedSelectorandQueryFilterPanelto handle array value selecting for MVTC