-
Notifications
You must be signed in to change notification settings - Fork 66
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
feat!: update Autosuggest component to better support freeform and selected values #2899
feat!: update Autosuggest component to better support freeform and selected values #2899
Conversation
✅ Deploy Preview for paragon-openedx ready!Built without sensitive environment variables
To edit notification comments on pull requests, go to your Netlify site configuration. |
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## release/chip-searchfield-pagination #2899 +/- ##
=======================================================================
+ Coverage 93.07% 93.18% +0.10%
=======================================================================
Files 249 249
Lines 4306 4342 +36
Branches 1029 1036 +7
=======================================================================
+ Hits 4008 4046 +38
+ Misses 294 292 -2
Partials 4 4 ☔ View full report in Codecov by Sentry. |
20084f0
to
b7b86aa
Compare
src/Form/FormAutosuggest.jsx
Outdated
/** Specifies if empty values trigger an error state */ | ||
valueRequired: PropTypes.bool, | ||
/** Informs user they must input a value. */ | ||
valueRequiredErrorMessageText: PropTypes.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 assume that there should be valueRequiredErrorMessageText
provided if valueRequired
equals true
. Please, consider:
import { requiredWhen } from '../utils/propTypes';
...
valueRequiredErrorMessageText: requiredWhen(PropTypes.string, 'valueRequired'),
And the same for selectionRequired
and selectionRequiredErrorMessageText
, customError
and customErrorMessageText
.
Another option, if no message is provided and there should only be a red box to indicate the error, I would remove the x
under the input, otherwise it will appear incomplete.
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 call! I was debating including default error messages, but I think using requiredWhen
is the cleanest solution!
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.
@monteri I have updated these to use requiredWhen
paragon/src/Form/FormAutosuggest.jsx
Lines 421 to 430 in f8a4e5e
/** Informs user they must input a value. */ | |
valueRequiredErrorMessageText: requiredWhen(PropTypes.string, 'valueRequired'), | |
/** Specifies if freeform values trigger an error state */ | |
selectionRequired: PropTypes.bool, | |
/** Informs user they must make a selection. */ | |
selectionRequiredErrorMessageText: requiredWhen(PropTypes.string, 'selectionRequired'), | |
/** Specifies the control is in a consumer provided error state */ | |
customError: PropTypes.bool, | |
/** Informs user of other errors. */ | |
customErrorMessageText: requiredWhen(PropTypes.string, 'customError'), |
d69e3d3
to
716a6dd
Compare
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.
@brian-smith-tcril leave some suggestions, please take a look
src/Form/FormAutosuggest.jsx
Outdated
const matchingDropdownItem = filteredItems.find((o) => ( | ||
o.props.children.toLowerCase() === userProvidedText.toLowerCase() | ||
)); | ||
if (!matchingDropdownItem) { |
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.
It may be worth adding spaces between constants, functions, and conditions. This will improve the readability of the code. Please let me know what your thoughts are.
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 added some spaces and comments in here, hopefully that clears things up a bit.
3d7ec48
to
f8a4e5e
Compare
})} | ||
value={value.userProvidedText} | ||
/> | ||
<Button onClick={forceUpdateErrorState}>Trigger validation</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.
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.
src/Form/form-autosuggest.mdx
Outdated
const [valueRequired, setValueRequired] = useState(false); | ||
const [selectionRequired, setSelectionRequired] = useState(false); | ||
const [customValidation, setCustomValidation] = useState(false); |
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: perhaps prefix these boolean state variables with isValueRequired
, isSelectionRequired
, hasCustomValidation
.
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.
src/Form/FormAutosuggest.jsx
Outdated
useImperativeHandle(ref, () => ({ | ||
updateErrorStateAndErrorMessage, | ||
})); |
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/suggestion] May be a good idea to include a comment here or elsewhere documenting the use of useImperativeHandle
to only include updateErrorStateAndErrorMessage
on its exposed methods. That way, future contributions will have some guidance on how to keep this maintained such that methods intended to be exposed are intentionally included here? 🤔
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 you have another suggestion for a comment I'm more than happy to change it!
src/Form/FormAutosuggest.jsx
Outdated
selectors: arrowKeyNavigationSelector, | ||
ignoredKeys: ignoredArrowKeysNames, | ||
}); | ||
const [dropdownExpanded, setDropdownExpanded] = useState(false); |
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: isDropdownExpanded
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.
src/Form/FormAutosuggest.jsx
Outdated
const [displayValue, setDisplayValue] = useState(() => { | ||
if (!value) { | ||
return ''; | ||
} | ||
|
||
setState(prevState => ({ | ||
...prevState, | ||
dropDownItems: [], | ||
displayValue: clickedValue, | ||
})); | ||
return value.userProvidedText; | ||
}); |
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: could this be simplified to:
const [displayValue, setDisplayValue] = useState(value?.userProvidedText || '');
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.
src/Form/FormAutosuggest.jsx
Outdated
// eslint-disable-next-line no-shadow | ||
const { children, onClick, ...rest } = child.props; |
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: to avoid // eslint-disable-next-line no-shadow
, consider renaming children
once deconstructed (i.e., const { children: childChildren, ... } = child.props;
to avoid the naming conflict with the parent children
.
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 (onSelected && clickedValue !== value) { | ||
onSelected(clickedValue); | ||
} | ||
const FormAutosuggest = forwardRef( |
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/suggestion/consideration] I'm wondering if it might make sense to abstract some of this logic into custom React hook(s) to keep the component code itself more focused on the UI presentation. For example, a custom hook(s) could return the callback functions to the component for use, having been defined in the hook(s).
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'd like to think about this one a bit. I worry that too much abstraction might lead to consumers using the component in ways that feel broken, but that could likely be resolved by providing good examples.
Maybe it makes sense to break this out into a separate issue to do some discovery on it?
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 suggestion to abstract some of the logic shouldn't really affect consumers? The custom hooks wouldn't be exposed to consumers; just for use internally within the component.
Generally when a components implementation reaches several hundred plus lines that feels like the component might have a bit too much going on where it could be simplified through the use of hooks to improve readability, etc.
Totally fine to defer and consider. Just a suggestion to make the component a bit more maintainable, where it makes sense.
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 custom hooks wouldn't be exposed to consumers; just for use internally within the component.
Gotcha, I misinterpreted the comment.
Totally fine to defer and consider. Just a suggestion to make the component a bit more maintainable, where it makes sense.
That sounds good. It's definitely a good idea, but I think it makes sense to hold off until after getting this PR merged.
f8a4e5e
to
9310aa0
Compare
…values This updates the Autosuggest component to provide the consumer with: * The value entered in the textbox by the user * The selected value (as it is displayed in the dropdown) * The id of the selected option This also allows the consumer to determine if the Autosuggest component should be in an error state when * No text has been entered * Text has been entered, but not selection has been made * An external validation check has failed - "customError" The consumer will provide error messages for the appropriate error states BREAKING CHANGE: value of Autosuggest component is now an object instead of a string BREAKING CHANGE: Autosuggest component now uses onChange instead of onSelected BREAKING CHANGE: Autosuggest component now takes in different error messages for value/selection required, and custom errors
9310aa0
to
e3daacf
Compare
8c09b4f
to
e187749
Compare
35485ef
into
openedx:release/chip-searchfield-pagination
Description
This updates the Autosuggest component to provide the consumer with:
This also allows the consumer to determine if the Autosuggest component should be in an error state when
The consumer will provide error messages for the appropriate error states
BREAKING CHANGE: value of Autosuggest component is now an object instead of a string
BREAKING CHANGE: Autosuggest component now uses onChange instead of onSelected
BREAKING CHANGE: Autosuggest component now takes in different error messages for value/selection required, and custom errors
Deploy Preview
https://deploy-preview-2899--paragon-openedx.netlify.app/components/form/form-autosuggest/
Merge Checklist
example
app?wittjeff
andadamstankiewicz
as reviewers on this PR.Post-merge Checklist