Skip to content

feat: add feedback prop to combobox #153

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.

Sign up for GitHub

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

Open
wants to merge 2 commits into
base: next
Choose a base branch
from
Open

feat: add feedback prop to combobox #153

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
54f4bb9
feat: add feedback prop to combobox
digitalsadhu Sep 27, 2022
8cf7cf1
refactor: split feedback into levels (info, warn and error)
digitalsadhu Nov 7, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 21 additions & 0 deletions packages/combobox/docs/Combobox.mdx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -308,6 +308,27 @@ function Example() {
}
```

### Providing feedback

You can provide feedback to the user via the feedback props. This is especially
useful when performing asynchronous operations such as fetching options from an
API as it enables you to tell the user that they need to wait for search results
or that no results were found and so on. Feedback can be provided using any of
the info, warning or error feedback props.

```ts
<Combobox
...
feedbackInfo="Søker..."
feedbackWarning="Ingen treff"
feedbackError="Vi fant desverre ikke adressen din akkurat nå. Prov på nytt"
Comment on lines +322 to +324
Copy link
Contributor

@BalbinaK BalbinaK Nov 7, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just wondering what you think about having a feedback prop that accepts a text string and feedbackType prop that accepts FeedbackType, e.g. "info" | "warning" (we can default to one that makes most sense). This would give us freedom to handle more feedback styles without adding more props or making breaking changes.

<Combobox
  ...
  feedback="Søker..."
  feedbackType="info"
/>

We could also try typing feedback as feedback?: { text: string; type?: "info" | "warning" } to not add more props to an already complex Combobox.

What do you think?

Copy link
Collaborator Author

@digitalsadhu digitalsadhu Nov 7, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I considered this and am happy to be persuaded that this is the better way to go. The whole thing is a bit awkward generally as you need to juggle more than 1 property when changing the feedback message during an async operation such as a search.

Copy link
Contributor

@BalbinaK BalbinaK Nov 7, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm. I guess you'd still need to change 2 properties, i.e. resetting the one that was there before and setting a new one.

Copy link
Collaborator Author

@digitalsadhu digitalsadhu Nov 7, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more. 

<Combobox
  ...
  feedback="Søker..."
  feedbackStyle="subtle"
/>
<Combobox
  ...
  feedback="Ingen treff"
  feedbackStyle="emphasis"
/>

Or we could lean into React's ability to take objects as props and do something like this:

<Combobox
  ...
  feedback={ message: "Ingen treff", style: "subtle" }
/>

??

Copy link
Contributor

@BalbinaK BalbinaK Nov 7, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a reason why we can't go with light & bold? I'm just wondering if users will understand what emphasis and subtle entail without testing them first.

Copy link
Contributor

@martinetruls martinetruls Nov 7, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this discussion is quite related to what @martin-storsletten has to say regards accessibility. If they should include some semantics it won't be sufficient to use style ad an indicator.

Copy link
Collaborator Author

@digitalsadhu digitalsadhu Nov 7, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My only concern with "light" and "bold" is that its highly coupled to how the styling is meaning if we change the styling, we might then need to change the name of the props. That said, that may not be very likely at all and light and bold may be easy to understand.

...
/>
```

When any of these props are set to a non empty string, options will not show.
Omit feedback props entirely or set them to empty strings when not in use.

## Combobox Props

```props packages/combobox/src/component.tsx
Expand Down
162 changes: 94 additions & 68 deletions packages/combobox/src/component.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,9 @@ export const Combobox = forwardRef<HTMLInputElement, ComboboxProps>(
onFocus,
onBlur,
optional,
feedbackInfo,
feedbackWarn,
feedbackError,
...rest
} = props;

Expand Down Expand Up @@ -249,77 +252,100 @@ export const Combobox = forwardRef<HTMLInputElement, ComboboxProps>(
{getAriaText(currentOptions, value)}
</span>

<div
hidden={!isOpen || !currentOptions.length}
className={classNames(
listClassName,
'absolute left-0 right-0 bg-primary pb-8 rounded-8 bg-white shadow',
)}
style={{
zIndex: 3, // Force popover above misc. page content (mobile safari issue)
}}
>
<ul
id={listboxId}
role="listbox"
className={classNames('m-0 p-0 select-none list-none', {
[MATCH_SEGMENTS_CLASS_NAME]: matchTextSegments,
})}
{feedbackInfo && (
<div className="absolute pb-8 left-0 right-0 bg-primary rounded-8 bg-white shadow">
<div id="static-text" className="block p-8 text-gray-500">
{feedbackInfo}
</div>
</div>
)}
{feedbackWarn && (
<div className="absolute pb-8 left-0 right-0 bg-primary rounded-8 bg-white shadow">
<div id="static-text" className="block p-8 font-bold">
{feedbackWarn}
</div>
</div>
)}
{feedbackError && (
Copy link
Contributor

@martinetruls martinetruls Nov 7, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this sufficient for screen readers, or does it need some aria attributes to highlight the error?

Copy link
Collaborator Author

@digitalsadhu digitalsadhu Nov 7, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Question for @martin-storsletten

Copy link
Collaborator Author

@digitalsadhu digitalsadhu Nov 7, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Might also depend on whether this is actually an error. I might be off base with that. Perhaps its just emphasised feedback text.

<div className="absolute pb-8 left-0 right-0 bg-primary rounded-8 bg-white shadow">
<div id="static-text" className="block p-8 font-bold">
{feedbackError}
</div>
</div>
)}
Comment on lines +262 to +275
Copy link
Contributor

@BalbinaK BalbinaK Nov 7, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need to have two different props if they end up with the same style?

Copy link
Collaborator Author

@digitalsadhu digitalsadhu Nov 7, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm honestly pretty unsure. They seemed like 3 distinct use cases even though they look the same so I leaned toward keeping them semantically different. We could go for something that leans more into the category of style instead of a "level" approach. Eg. light and heavy or something.

Copy link
Collaborator Author

@digitalsadhu digitalsadhu Nov 7, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could we go with "subtle" (as seen in "Søker...") and "emphasis" (as seen in "Ingen treff")?

{!feedbackInfo && !feedbackWarn && !feedbackError && (
<div
hidden={!isOpen || !currentOptions.length}
className={classNames(
listClassName,
'absolute left-0 right-0 bg-primary pb-8 rounded-8 bg-white shadow',
)}
style={{
zIndex: 3, // Force popover above misc. page content (mobile safari issue)
}}
>
{currentOptions.map((option) => {
const display = option.label || option.value;
let match: ReactNode = [];

if (matchTextSegments && !highlightValueMatch) {
const startIdx = display
.toLowerCase()
.indexOf(option.currentInputValue.toLowerCase());

if (startIdx !== -1) {
const endIdx = startIdx + option.currentInputValue.length;
match = (
<>
{display.substring(0, startIdx)}
<span data-combobox-text-match className="font-bold">
{display.substring(startIdx, endIdx)}
</span>
{display.substring(endIdx)}
</>
);
} else {
match = <span>{display}</span>;
<ul
id={listboxId}
role="listbox"
className={classNames('m-0 p-0 select-none list-none', {
[MATCH_SEGMENTS_CLASS_NAME]: matchTextSegments,
})}
>
{currentOptions.map((option) => {
const display = option.label || option.value;
let match: ReactNode = [];

if (matchTextSegments && !highlightValueMatch) {
const startIdx = display
.toLowerCase()
.indexOf(option.currentInputValue.toLowerCase());

if (startIdx !== -1) {
const endIdx = startIdx + option.currentInputValue.length;
match = (
<>
{display.substring(0, startIdx)}
<span data-combobox-text-match className="font-bold">
{display.substring(startIdx, endIdx)}
</span>
{display.substring(endIdx)}
</>
);
} else {
match = <span>{display}</span>;
}
} else if (highlightValueMatch) {
match = highlightValueMatch(display, value);
}
} else if (highlightValueMatch) {
match = highlightValueMatch(display, value);
}

return (
<li
key={option.id}
id={option.id}
role="option"
aria-selected={navigationOption?.id === option.id}
tabIndex={-1}
onClick={() => {
new Promise((res) => res(setNavigationOption(option))).then(
() => {

return (
<li
key={option.id}
id={option.id}
role="option"
aria-selected={navigationOption?.id === option.id}
tabIndex={-1}
onClick={() => {
new Promise((res) =>
res(setNavigationOption(option)),
).then(() => {
handleSelect(option);
},
);
}}
className={classNames({
[`block cursor-pointer p-8 hover:bg-${OPTION_HIGHLIGHT_COLOR} ${OPTION_CLASS_NAME}`]:
true,
[`bg-${OPTION_HIGHLIGHT_COLOR}`]:
navigationOption?.id === option.id,
})}
>
{matchTextSegments || highlightValueMatch ? match : display}
</li>
);
})}
</ul>
</div>
});
}}
className={classNames({
[`block cursor-pointer p-8 hover:bg-${OPTION_HIGHLIGHT_COLOR} ${OPTION_CLASS_NAME}`]:
true,
[`bg-${OPTION_HIGHLIGHT_COLOR}`]:
navigationOption?.id === option.id,
})}
>
{matchTextSegments || highlightValueMatch ? match : display}
</li>
);
})}
</ul>
</div>
)}
</div>
);
},
Expand Down
9 changes: 9 additions & 0 deletions packages/combobox/src/props.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -117,6 +117,15 @@ export type ComboboxProps = {

/** Whether to show optional text */
optional?: boolean;

/** Feedback string to use to inform users about something. Eg. Show "Søker..." feedback to users as they type. */
feedbackInfo?: string;

/** Feedback string to use to warn users about something. Eg. if there are no results when searching. */
feedbackWarn?: string;

/** Feedback string to show users if there is an error as they interact with the combobox. */
feedbackError?: string;
} & Omit<
React.PropsWithoutRef<JSX.IntrinsicElements['input']>,
'onChange' | 'type' | 'value' | 'label'
Expand Down
103 changes: 103 additions & 0 deletions packages/combobox/stories/Combobox.stories.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -356,3 +356,106 @@ export const Optional = () => {
</>
);
};

export const Searching = () => {
return (
<Combobox
label="Star Wars character"
disableStaticFiltering
matchTextSegments
openOnFocus
value="asd"
feedbackInfo='Søker...'
onChange={(val) => console.log('change')}
options={[
{ value: 'Product manager' },
{ value: 'Produktledelse' },
{ value: 'Prosessoperatør' },
{ value: 'Prosjekteier' },
]}
/>
);
};

export const AsyncFetchWithFeedback = () => {
const [query, setQuery] = React.useState('');
const [value, setValue] = React.useState('');
const [infoFeedback, setInfoFeedback] = React.useState('');
const [warningFeedback, setWarningFeedback] = React.useState('');
const [errorFeedback, setErrorFeedback] = React.useState('');
const characters = useDebouncedSearch(query, 300);

// Generic debouncer
function useDebouncedSearch(query, delay) {
const [characters, setCharacters] = React.useState([]);

React.useEffect(() => {
if (!query.length) {
setCharacters([]);
return;
}

const handler = setTimeout(async () => {
setInfoFeedback('Søker...');
setWarningFeedback('');
setErrorFeedback('');
try {
const res = await fetch('https://swapi.dev/api/people/?search=' + query.trim())
const { results } = await res.json();
console.log('Results from API', query);
if (!results.length) {
setWarningFeedback('Ingen treff');
}
setCharacters(results.map((c) => ({ value: c.name })));
} catch(err) {
setErrorFeedback('API Fail');
} finally {
setInfoFeedback('');
}
}, delay);

return () => {
clearTimeout(handler);
};
}, [delay, query]);

return characters;
}

return (
<Combobox
label="Star Wars character"
disableStaticFiltering
matchTextSegments
openOnFocus
value={value}
onChange={(val) => {
setValue(val);
setQuery(val);
}}
onSelect={(val) => {
setValue(val);
action('select')(val);
}}
onBlur={() => {
setInfoFeedback('');
setWarningFeedback('');
setErrorFeedback('');
}}
options={characters}
feedbackInfo={infoFeedback}
feedbackWarn={warningFeedback}
feedbackError={errorFeedback}
>
<Affix
suffix
clear
aria-label="Clear text"
onClick={() => {
setValue('');
setQuery('');
}}
/>
</Combobox>
);
};