BAH 4610 | Migrated AutoComplete components family #35
Conversation
sahupadma
changed the title
ravinderkabli
left a comment
ravinderkabli
left a comment
There was a problem hiding this comment.
Review for BAH-4610
Good foundation for the AutoComplete Carbon migration — the test suite is comprehensive (768 lines), the propsRef stale-closure pattern is correctly applied, and CarbonContainer wiring works for the main autocomplete / CodedControl path.
Found 4 blocking issue(s), 8 suggestion(s), and 1 nit(s).
Acceptance Criteria: 13/20 met, 5 partial, 2 missing (AC#6: fires undefined not null; AC#15: no onBlur).
| invalid={hasError} | ||
| items={safeOptions.length > 0 ? safeOptions : safePropOptions} | ||
| itemToString={(item) => (item ? item[labelKey] || '' : '')} | ||
| initialSelectedItems={initialSelectedItems} |
There was a problem hiding this comment.
blocking: Carbon FilterableMultiSelect only accepts initialSelectedItems as seed state — it does not respond to prop changes after mount. When propValue is cleared externally (form reset, AddMore row removal), setValue(propValue) runs but the widget stays unchanged. The single-select ComboBox correctly uses the controlled selectedItem prop.
Fix: add a key prop derived from the value identity (e.g. key={JSON.stringify(propValue)}) to force remount when the external value changes, or use MultiSelect if that version supports a controlled selectedItems prop.
| ); | ||
| } | ||
|
|
||
| if (asynchronous) { |
There was a problem hiding this comment.
suggestion: No onBlur prop passed to ComboBox — AC #15 requires onBlur to fire when the user leaves the input. Carbon's ComboBox supports onBlur.
Fix: add onBlur: PropTypes.func to propTypes and pass onBlur={onBlur} on both ComboBox renders.
| const { properties } = this.props; | ||
| const isSearchable = (properties.style === 'autocomplete'); | ||
| const minimumInput = isSearchable ? 2 : 0; | ||
| const isLoading = this.state.providerData.length === 0 && this.props.value; |
There was a problem hiding this comment.
suggestion: isLoading = this.state.providerData.length === 0 && this.props.value is true when the fetch fails (the catch block calls showNotification but never updates providerData). In edit mode with a pre-populated value, the spinner never stops and AutoComplete is never rendered.
Fix: add a loaded: false flag in state, set to true in both .then() and .catch(). Use isLoading = !this.state.loaded && this.props.value.
| setValue(undefined); | ||
| setHasError(hasErrors(errors)); | ||
| if (onValueChange) { | ||
| onValueChange(undefined, errors); |
There was a problem hiding this comment.
nit: onValueChange(undefined, errors) — AC #6 specifies the clear callback fires with null. If any parent discriminates null vs undefined, clears will be mishandled.
Consider changing to onValueChange(null, errors) in both clear branches (here and line 180), or confirm with the team that undefined is acceptable.
3 participants
JIRA - https://bahmni.atlassian.net/browse/BAH-4610
Migrated AutoComplete components family to bahmni-design-system Carbon Component.
Location and Provider used AutoComplete for rendering migrated that also.