feat!: update Autosuggest component to better support freeform and selected values by brian-smith-tcril · Pull Request #2899 · openedx/paragon

brian-smith-tcril · 2023-12-07T02:26:45Z

Description

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

Deploy Preview

https://deploy-preview-2899--paragon-openedx.netlify.app/components/form/form-autosuggest/

Merge Checklist

If your update includes visual changes, have they been reviewed by a designer? Send them a link to the Netlify deploy preview, if applicable.
Does your change adhere to the documented style conventions?
Do any prop types have missing descriptions in the Props API tables in the documentation site (check deploy preview)?
Were your changes tested using all available themes (see theme switcher in the header of the deploy preview, under the "Settings" icon)?
Were your changes tested in the example app?
Is there adequate test coverage for your changes?
Consider whether this change needs to reviewed/QA'ed for accessibility (a11y). If so, please add wittjeff and adamstankiewicz as reviewers on this PR.

Post-merge Checklist

Verify your changes were released to NPM at the expected version.
If you'd like, share your contribution in #show-and-tell.
🎉 🙌 Celebrate! Thanks for your contribution.

netlify · 2023-12-07T02:26:50Z

✅ Deploy Preview for paragon-openedx ready!

Built without sensitive environment variables

Name	Link
🔨 Latest commit	`e3daacf`
🔍 Latest deploy log	https://app.netlify.com/sites/paragon-openedx/deploys/65a6f936ce5aef0007cfe4fc
😎 Deploy Preview	https://deploy-preview-2899--paragon-openedx.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

codecov · 2023-12-07T02:37:10Z

Codecov Report

Attention: 1 lines in your changes are missing coverage. Please review.

Comparison is base (8c09b4f) 93.07% compared to head (e3daacf) 93.18%.

Files	Patch %	Lines
src/Form/FormAutosuggest.jsx	99.28%	1 Missing ⚠️

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.
📢 Have feedback on the report? Share it here.

monteri · 2023-12-08T09:07:53Z

+  /** Specifies if empty values trigger an error state */
+  valueRequired: PropTypes.bool,
+  /** Informs user they must input a value. */
+  valueRequiredErrorMessageText: PropTypes.string,


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.

Good call! I was debating including default error messages, but I think using requiredWhen is the cleanest solution!

@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'),

peterkulko

@brian-smith-tcril leave some suggestions, please take a look

peterkulko · 2023-12-11T08:58:13Z

+    const matchingDropdownItem = filteredItems.find((o) => (
+      o.props.children.toLowerCase() === userProvidedText.toLowerCase()
+    ));
+    if (!matchingDropdownItem) {


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.

I added some spaces and comments in here, hopefully that clears things up a bit.

…2842)

adamstankiewicz · 2024-01-16T19:03:45Z

+          })}
+          value={value.userProvidedText}
+        />
+        <Button onClick={forceUpdateErrorState}>Trigger validation</Button>


nit: consider using Stack or CSS utility class to give some vertical whitespace between the Form.Control and the Button.

addressed here https://github.com/openedx/paragon/compare/f8a4e5ee118199936c3ff6653e3dfb389d12fffa..9310aa04d988eba1ee524cf920e5ad54dd39590f#diff-3ce694685ee1cc2b274e86b07ff39240bff96ddf3862d4c5366d0f24ad863d38R81-R91

adamstankiewicz · 2024-01-16T19:06:09Z

+  const [valueRequired, setValueRequired] = useState(false);
+  const [selectionRequired, setSelectionRequired] = useState(false);
+  const [customValidation, setCustomValidation] = useState(false);


nit: perhaps prefix these boolean state variables with isValueRequired, isSelectionRequired, hasCustomValidation.

addressed throughout https://github.com/openedx/paragon/compare/f8a4e5ee118199936c3ff6653e3dfb389d12fffa..9310aa04d988eba1ee524cf920e5ad54dd39590f

adamstankiewicz · 2024-01-16T19:10:08Z

+    useImperativeHandle(ref, () => ({
+      updateErrorStateAndErrorMessage,
+    }));


[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? 🤔

added a comment here https://github.com/openedx/paragon/compare/f8a4e5ee118199936c3ff6653e3dfb389d12fffa..9310aa04d988eba1ee524cf920e5ad54dd39590f#diff-96e13c1a773ec8d218ea5667d3dec6c8f6eeadcb00c81584f50d6c3d4d52135fR170-R171

If you have another suggestion for a comment I'm more than happy to change it!

adamstankiewicz · 2024-01-16T19:11:44Z

+      selectors: arrowKeyNavigationSelector,
+      ignoredKeys: ignoredArrowKeysNames,
+    });
+    const [dropdownExpanded, setDropdownExpanded] = useState(false);


nit: isDropdownExpanded

addressed https://github.com/openedx/paragon/compare/f8a4e5ee118199936c3ff6653e3dfb389d12fffa..9310aa04d988eba1ee524cf920e5ad54dd39590f#diff-96e13c1a773ec8d218ea5667d3dec6c8f6eeadcb00c81584f50d6c3d4d52135fR45

adamstankiewicz · 2024-01-16T19:12:33Z

+    const [displayValue, setDisplayValue] = useState(() => {
+      if (!value) {
+        return '';
+      }

-    setState(prevState => ({
-      ...prevState,
-      dropDownItems: [],
-      displayValue: clickedValue,
-    }));
+      return value.userProvidedText;
+    });


nit: could this be simplified to:

const [displayValue, setDisplayValue] = useState(value?.userProvidedText || '');

addressed https://github.com/openedx/paragon/compare/f8a4e5ee118199936c3ff6653e3dfb389d12fffa..9310aa04d988eba1ee524cf920e5ad54dd39590f#diff-96e13c1a773ec8d218ea5667d3dec6c8f6eeadcb00c81584f50d6c3d4d52135fR49

adamstankiewicz · 2024-01-16T19:18:01Z

+      // eslint-disable-next-line no-shadow
+        const { children, onClick, ...rest } = child.props;


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.

addressed https://github.com/openedx/paragon/compare/f8a4e5ee118199936c3ff6653e3dfb389d12fffa..9310aa04d988eba1ee524cf920e5ad54dd39590f#diff-96e13c1a773ec8d218ea5667d3dec6c8f6eeadcb00c81584f50d6c3d4d52135fR90

adamstankiewicz · 2024-01-16T19:21:44Z

-    if (onSelected && clickedValue !== value) {
-      onSelected(clickedValue);
-    }
+const FormAutosuggest = forwardRef(


[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).

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?

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.

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.

…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

brian-smith-tcril force-pushed the autosuggest-rework branch from 20084f0 to b7b86aa Compare December 7, 2023 02:46

brian-smith-tcril changed the title ~~update Autosuggest component to better support freeform and selected values~~ feat!: update Autosuggest component to better support freeform and selected values Dec 7, 2023

monteri reviewed Dec 8, 2023

View reviewed changes

brian-smith-tcril force-pushed the autosuggest-rework branch 2 times, most recently from d69e3d3 to 716a6dd Compare December 8, 2023 20:30

brian-smith-tcril marked this pull request as ready for review December 8, 2023 20:51

peterkulko reviewed Dec 11, 2023

View reviewed changes

brian-smith-tcril mentioned this pull request Dec 13, 2023

[Tagging] Assign taxonomy to organizations openedx/modular-learning#117

Closed

brian-smith-tcril force-pushed the autosuggest-rework branch 7 times, most recently from 3d7ec48 to f8a4e5e Compare December 19, 2023 23:53

peterkulko and others added 5 commits December 28, 2023 09:46

feat!: Chip component redesign (openedx#2836)

34720f7

refactor!: refactoring Pagination component (openedx#2837)

9460520

fix: hover for danger button (openedx#2844)

464c949

feat!: replace custom submit and reset btns with IconButton (openedx#…

a2b53d0

…2842)

fix: add missing li to dropdown in pagination

8c09b4f

rpenido mentioned this pull request Jan 11, 2024

feat: assign taxonomy to organizations [FC-0036] openedx/frontend-app-authoring#760

Merged

adamstankiewicz approved these changes Jan 16, 2024

View reviewed changes

brian-smith-tcril force-pushed the autosuggest-rework branch from f8a4e5e to 9310aa0 Compare January 16, 2024 21:21

brian-smith-tcril force-pushed the autosuggest-rework branch from 9310aa0 to e3daacf Compare January 16, 2024 21:46

brian-smith-tcril changed the base branch from master to release/chip-searchfield-pagination January 16, 2024 21:46

adamstankiewicz approved these changes Jan 18, 2024

View reviewed changes

adamstankiewicz force-pushed the release/chip-searchfield-pagination branch from 8c09b4f to e187749 Compare January 18, 2024 17:57

adamstankiewicz merged commit 35485ef into openedx:release/chip-searchfield-pagination Jan 18, 2024

This was referenced Feb 12, 2024

feat: autosuggest error handling #2652

Closed

form autosuggest option - user facing vs internal values #2634

Closed

	/** 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'),

		// eslint-disable-next-line no-shadow
		const { children, onClick, ...rest } = child.props;

Uh oh!

Conversation

brian-smith-tcril commented Dec 7, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Deploy Preview

Merge Checklist

Post-merge Checklist

Uh oh!

netlify Bot commented Dec 7, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

✅ Deploy Preview for paragon-openedx ready!

Uh oh!

codecov Bot commented Dec 7, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

peterkulko left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

adamstankiewicz Jan 16, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

brian-smith-tcril commented Dec 7, 2023 •

edited

Loading

netlify Bot commented Dec 7, 2023 •

edited

Loading

codecov Bot commented Dec 7, 2023 •

edited

Loading

adamstankiewicz Jan 16, 2024 •

edited

Loading