Skip to content

Combobox: add translations and adjust buttons #3409

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

Merged
merged 9 commits into from
Dec 2, 2024
Merged
5 changes: 5 additions & 0 deletions .changeset/cool-items-shave.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"@navikt/ds-react": patch
---

Combobox: Hid buttons from screen readers, added `title` on clear button, and deprecated prop `toggleListButtonLabel`.
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ import cl from "clsx";
import React from "react";
import { PlusIcon } from "@navikt/aksel-icons";
import { BodyShort, Label } from "../../../typography";
import { useI18n } from "../../../util/i18n/i18n.context";
import { useInputContext } from "../Input/Input.context";
import { useSelectedOptionsContext } from "../SelectedOptions/selectedOptionsContext";
import { isInList, toComboboxOption } from "../combobox-utils";
Expand All @@ -22,6 +23,7 @@ const AddNewOption = () => {
} = useFilteredOptionsContext();
const { isMultiSelect, selectedOptions, toggleOption } =
useSelectedOptionsContext();
const translate = useI18n("Combobox");
return (
<li
tabIndex={-1}
Expand Down Expand Up @@ -51,7 +53,7 @@ const AddNewOption = () => {
>
<PlusIcon aria-hidden />
<BodyShort size={size}>
Legg til{" "}
{translate("addOption")}{" "}
<Label as="span" size={size}>
&#8220;{searchTerm}&#8221;
</Label>
Expand Down
Original file line number Diff line number Diff line change
@@ -1,18 +1,20 @@
import React from "react";
import { Loader } from "../../../loader";
import { useI18n } from "../../../util/i18n/i18n.context";
import { useInputContext } from "../Input/Input.context";
import filteredOptionsUtil from "./filtered-options-util";

const LoadingMessage = () => {
const {
inputProps: { id },
} = useInputContext();
const translate = useI18n("Combobox");
return (
<div
className="navds-combobox__list-item--loading"
id={filteredOptionsUtil.getIsLoadingId(id)}
>
<Loader title="Søker..." />
<Loader title={translate("loading")} />
</div>
);
};
Expand Down
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import React from "react";
import { useI18n } from "../../../util/i18n/i18n.context";
import { useInputContext } from "../Input/Input.context";
import { useSelectedOptionsContext } from "../SelectedOptions/selectedOptionsContext";
import filteredOptionsUtil from "./filtered-options-util";
Expand All @@ -8,6 +9,10 @@ const MaxSelectedMessage = () => {
inputProps: { id },
} = useInputContext();
const { maxSelected, selectedOptions } = useSelectedOptionsContext();
const translate = useI18n(
"Combobox",
maxSelected?.message ? { maxSelected: maxSelected.message } : undefined,
);

if (!maxSelected) {
return null;
Expand All @@ -18,8 +23,10 @@ const MaxSelectedMessage = () => {
className="navds-combobox__list-item--max-selected"
id={filteredOptionsUtil.getMaxSelectedOptionsId(id)}
>
{maxSelected.message ??
`${selectedOptions.length} av ${maxSelected.limit} er valgt.`}
{translate("maxSelected", {
selected: selectedOptions.length,
limit: maxSelected.limit,
})}
</div>
);
};
Expand Down
13 changes: 9 additions & 4 deletions @navikt/core/react/src/form/combobox/Input/InputController.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ import cl from "clsx";
import React, { forwardRef } from "react";
import { XMarkIcon } from "@navikt/aksel-icons";
import { useMergeRefs } from "../../../util/hooks";
import { useI18n } from "../../../util/i18n/i18n.context";
import { useFilteredOptionsContext } from "../FilteredOptions/filteredOptionsContext";
import SelectedOptions from "../SelectedOptions/SelectedOptions";
import { useSelectedOptionsContext } from "../SelectedOptions/selectedOptionsContext";
Expand All @@ -11,7 +12,6 @@ import Input from "./Input";
import { useInputContext } from "./Input.context";
import ToggleListButton from "./ToggleListButton";

/* eslint-disable jsx-a11y/click-events-have-key-events */
export const InputController = forwardRef<
HTMLInputElement,
Omit<
Expand Down Expand Up @@ -53,7 +53,13 @@ export const InputController = forwardRef<

const mergedInputRef = useMergeRefs(inputRef, ref);

const translate = useI18n(
"Combobox",
clearButtonLabel ? { clear: clearButtonLabel } : undefined,
);

return (
// eslint-disable-next-line jsx-a11y/click-events-have-key-events
<div
className={cl("navds-combobox__wrapper-inner navds-text-field__input", {
"navds-combobox__wrapper-inner--virtually-unfocused":
Expand Down Expand Up @@ -88,10 +94,9 @@ export const InputController = forwardRef<
onClick={clearInput}
className="navds-combobox__button-clear"
tabIndex={-1}
aria-hidden
Copy link
Contributor Author

@HalvorHaugan HalvorHaugan Dec 2, 2024

Choose a reason for hiding this comment

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

aria-hidden on button results in this error message in the console in Chrome:

Blocked aria-hidden on an element because its descendant retained focus. The focus must not be hidden from assistive technology users. Avoid using aria-hidden on a focused element or its ancestor. Consider using the inert attribute instead, which will also prevent focus. For more details, see the aria-hidden section of the WAI-ARIA specification at https://w3c.github.io/aria/#aria-hidden.
Element with focus: button

Wondering if I should use a div instead? 🤔

Copy link
Collaborator

@KenAJoh KenAJoh Dec 2, 2024

Choose a reason for hiding this comment

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

Since this and ToggleListButton are both aria-hidden, can the wrapper-div get aria-hidden to solve this?
Screenshot 2024-12-02 at 09 27 43

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Doesn't help

Copy link
Collaborator

Choose a reason for hiding this comment

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

Is there actually a reason to hide this element? In Search we opt to not hide it, so is there an actual difference in expected UX here?

title={translate("clear")}
>
<span className="navds-sr-only">
{clearButtonLabel ? clearButtonLabel : "Tøm"}
</span>
<XMarkIcon aria-hidden />
</button>
)}
Expand Down
11 changes: 5 additions & 6 deletions @navikt/core/react/src/form/combobox/Input/ToggleListButton.tsx
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@it-vegard Please have a look at the event changes I made here. I removed onKeyDown since that shouldn't work anyways. I also changed onPointerUp to onClick since it should work the same way, but is better practice. Do you think this is ok?

Original file line number Diff line number Diff line change
Expand Up @@ -13,22 +13,21 @@ export const ToggleListButton = forwardRef<
>(({ toggleListButtonLabel }, ref) => {
const { isListOpen, toggleIsListOpen } = useFilteredOptionsContext();
const { focusInput } = useInputContext();

return (
<button
ref={ref}
type="button"
onPointerUp={() => {
onClick={() => {
toggleIsListOpen();
focusInput();
}}
onKeyDown={({ key }) => key === "Enter" && toggleIsListOpen()}
className="navds-combobox__button-toggle-list"
aria-expanded={isListOpen}
tabIndex={-1}
ref={ref}
aria-hidden
title={toggleListButtonLabel}
>
<span className="navds-sr-only">
{toggleListButtonLabel ?? "Alternativer"}
Copy link
Collaborator

Choose a reason for hiding this comment

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

"Alternativer" doesn't tell me what the action does here. Would "open alternatives", "open optionlist" or something in that world make sense?

Copy link
Contributor Author

@HalvorHaugan HalvorHaugan Nov 27, 2024

Choose a reason for hiding this comment

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

I don't think we need this text, so I just removed it and deprecated toggleListButtonLabel. No one is currently using that prop, so maybe we can just remove it right away?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@it-vegard @KenAJoh Should we remove the prop toggleListButtonLabel entirely instead of just marking it as deprecated?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Do it! Love removing unused props where possible ❤️

</span>
{isListOpen ? (
<ChevronUpIcon aria-hidden />
) : (
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,12 +14,6 @@ const Option = ({ option }: { option: ComboboxOption }) => {
const { isMultiSelect, removeSelectedOption } = useSelectedOptionsContext();
const { focusInput, readOnly, inputProps } = useInputContext();

const onClick = (e) => {
e.stopPropagation();
removeSelectedOption(option);
focusInput();
};

if (!isMultiSelect) {
return (
<div className="navds-combobox__selected-options--no-bg">
Expand All @@ -28,12 +22,24 @@ const Option = ({ option }: { option: ComboboxOption }) => {
);
}

return readOnly || inputProps.disabled ? (
<Chips.Toggle variant="neutral" checkmark={false} as="div">
if (readOnly || inputProps.disabled) {
return (
<Chips.Toggle variant="neutral" checkmark={false} as="div">
{option.label}
</Chips.Toggle>
);
}

return (
<Chips.Removable
onClick={(event) => {
event.stopPropagation();
removeSelectedOption(option);
focusInput();
}}
>
{option.label}
</Chips.Toggle>
) : (
<Chips.Removable onClick={onClick}>{option.label}</Chips.Removable>
</Chips.Removable>
);
};

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ import { act, render, screen } from "@testing-library/react";
import userEvent from "@testing-library/user-event";
import React, { useId } from "react";
import { describe, expect, test, vi } from "vitest";
import nb from "../../../util/i18n/locales/nb";
import { ComboboxProps, UNSAFE_Combobox } from "../index";

const options = [
Expand Down Expand Up @@ -73,7 +74,7 @@ describe("Render combobox", () => {
test("Should show loading icon when loading (used for async search)", async () => {
render(<App options={[]} isListOpen isLoading />);

expect(await screen.findByText("Søker...")).toBeInTheDocument();
expect(await screen.findByText(nb.Combobox.loading)).toBeInTheDocument();
});

test("Should not select previous focused element when closes", async () => {
Expand Down
17 changes: 12 additions & 5 deletions @navikt/core/react/src/form/combobox/combobox.stories.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -38,13 +38,14 @@ const options = [
"grapefruit",
];

export const Default: StoryFn<ComboboxProps & { maxSelected?: number }> = ({
maxSelected,
...rest
}) => (
export const Default: StoryFn<
ComboboxProps & { maxSelected?: number; maxSelectedMessage?: string }
> = ({ maxSelected, maxSelectedMessage, ...rest }) => (
<UNSAFE_Combobox
{...rest}
maxSelected={maxSelected && { limit: maxSelected }}
maxSelected={
maxSelected && { limit: maxSelected, message: maxSelectedMessage }
}
id="combobox"
/>
);
Expand Down Expand Up @@ -76,11 +77,17 @@ Default.argTypes = {
maxSelected: {
control: { type: "number" },
},
maxSelectedMessage: {
control: { type: "text" },
},
size: {
options: ["medium", "small"],
defaultValue: "medium",
control: { type: "radio" },
},
clearButtonLabel: {
control: { type: "text" },
},
};

export const WithPlaceholder: StoryFunction = () => {
Expand Down
6 changes: 3 additions & 3 deletions @navikt/core/react/src/form/combobox/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -132,7 +132,8 @@ export interface ComboboxProps
*/
limit: number;
/**
* Override the message to display when the limit for maximum selected options has been reached
* Message to display when the limit for maximum selected options has been reached
* @default "{selected} av maks {limit} er valgt."
*/
message?: string;
};
Expand All @@ -156,8 +157,7 @@ export interface ComboboxProps
toggleListButton?: boolean;
/**
* Custom name for the toggle list-button. Requires "toggleListButton" to be `true`.
*
* @default "Alternativer"
* @deprecated The name will be removed in a future major release.
*/
toggleListButtonLabel?: string;
/**
Expand Down
6 changes: 6 additions & 0 deletions @navikt/core/react/src/util/i18n/locales/en.ts
Original file line number Diff line number Diff line change
Expand Up @@ -117,4 +117,10 @@ export default {
closeDatePicker: "Close date picker",
closeMonthPicker: "Close month picker",
},
Combobox: {
addOption: "Add",
loading: "Searching…",
maxSelected: "{selected} of max {limit} are selected.",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I took the liberty of adding "max" to the text to make it clearer.

clear: "Clear",
},
} satisfies Translations;
8 changes: 8 additions & 0 deletions @navikt/core/react/src/util/i18n/locales/nb.ts
Original file line number Diff line number Diff line change
Expand Up @@ -122,4 +122,12 @@ export default {
closeDatePicker: "Lukk datovelger",
closeMonthPicker: "Lukk månedsvelger",
},
Combobox: {
/** The input value will be appended to the end of this text, e.g. `Legg til "input value"`. */
addOption: "Legg til",
/** Loader title */
loading: "Søker…",
maxSelected: "{selected} av maks {limit} er valgt.",
clear: "Tøm",
},
} satisfies TranslationMap;
6 changes: 6 additions & 0 deletions @navikt/core/react/src/util/i18n/locales/nn.ts
Original file line number Diff line number Diff line change
Expand Up @@ -117,4 +117,10 @@ export default {
closeDatePicker: "Lukk datoveljar",
closeMonthPicker: "Lukk månadsveljar",
},
Combobox: {
addOption: "Legg til",
loading: "Søker…",
maxSelected: "{selected} av maks {limit} er valt.",
clear: "Tøm",
},
} satisfies Translations;
Loading