Skip to content
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

Combobox: 🚸 hide caret on select (until options list opened) #3071

Merged
merged 20 commits into from
Sep 2, 2024
Merged
Show file tree
Hide file tree
Changes from 13 commits
Commits
Show all changes
20 commits
Select commit Hold shift + click to select a range
19ffcd0
:children_crossing: hide caret on select (until list opened)
JulianNymark Aug 6, 2024
ead066e
:fire: remove console.log()
JulianNymark Aug 6, 2024
10049d5
:art: invert varname for simpler CSS
JulianNymark Aug 6, 2024
d0fda3e
:children_crossing: also enable caret on "re-click" (while still focu…
JulianNymark Aug 6, 2024
463de66
:children_crossing: DX improvement, placeholders++
JulianNymark Aug 6, 2024
19bf759
:children_crossing: (combobox) always show caret for multiple select
JulianNymark Aug 7, 2024
ad3e1df
:children_crossing: hide caret on multiselect + isLimitReached
JulianNymark Aug 7, 2024
e11d8b2
:fire: remove console.log
JulianNymark Aug 7, 2024
61037de
:bug: hide caret when single has a selection (combobox)
JulianNymark Aug 9, 2024
717223a
:bug: hide caret if isLimitReached
JulianNymark Aug 9, 2024
009c5f4
:art: remove unnecessary useState()
JulianNymark Aug 12, 2024
b978ba1
:bug: correct deps for useEffect
JulianNymark Aug 12, 2024
99d0795
:bug: add maxSelected as dep
JulianNymark Aug 16, 2024
46048a1
Update @navikt/core/react/src/form/combobox/FilteredOptions/filteredO…
JulianNymark Aug 16, 2024
8de53dc
changeset
JulianNymark Aug 16, 2024
3e54582
:bug: avoid setting placeholder twice
JulianNymark Aug 19, 2024
fe51966
Update .changeset/fresh-boxes-enjoy.md
JulianNymark Aug 26, 2024
bdbd9f3
Merge branch 'main' into combobox-hide-caret-on-select
JulianNymark Aug 26, 2024
34b8527
:fire: remove redundant placeholder
JulianNymark Aug 26, 2024
4702bfc
Merge branch 'main' into combobox-hide-caret-on-select
JulianNymark Aug 28, 2024
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
4 changes: 4 additions & 0 deletions @navikt/core/css/form/combobox.css
Original file line number Diff line number Diff line change
Expand Up @@ -104,6 +104,10 @@
height: var(--__ac-combobox-input-height);
}

.navds-combobox__input--hide-caret {
caret-color: transparent;
}

.navds-combobox__input:focus-visible {
outline: none;
border: none;
Expand Down
JulianNymark marked this conversation as resolved.
Show resolved Hide resolved
JulianNymark marked this conversation as resolved.
Show resolved Hide resolved
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,7 @@ const FilteredOptionsProvider = ({
setValue,
setSearchTerm,
shouldAutocomplete,
setHideCaret,
} = useInputContext();
const { maxSelected } = useSelectedOptionsContext();

Expand Down Expand Up @@ -136,9 +137,12 @@ const FilteredOptionsProvider = ({
const toggleIsListOpen = useCallback(
(newState?: boolean) => {
virtualFocus.moveFocusToTop();
if (newState ?? !isInternalListOpen) {
setHideCaret(!!maxSelected?.isLimitReached);
}
setInternalListOpen((oldState) => newState ?? !oldState);
},
[virtualFocus],
[virtualFocus, maxSelected, isInternalListOpen, setHideCaret],
JulianNymark marked this conversation as resolved.
Show resolved Hide resolved
);

const isValueNew = useMemo(
Expand Down
5 changes: 5 additions & 0 deletions @navikt/core/react/src/form/combobox/Input/Input.context.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,8 @@ interface InputContextValue extends FormFieldType {
setSearchTerm: React.Dispatch<React.SetStateAction<string>>;
shouldAutocomplete?: boolean;
toggleOpenButtonRef: React.RefObject<HTMLButtonElement>;
hideCaret: boolean;
setHideCaret: React.Dispatch<React.SetStateAction<boolean>>;
}

const [InputContextProvider, useInputContext] =
Expand Down Expand Up @@ -69,6 +71,7 @@ const InputProvider = ({ children, value: props }: Props) => {
const inputRef = useRef<HTMLInputElement | null>(null);
const toggleOpenButtonRef = useRef<HTMLButtonElement>(null);
const [internalValue, setInternalValue] = useState<string>(defaultValue);
const [hideCaret, setHideCaret] = useState(false);

const value = useMemo(
() => String(externalValue ?? internalValue),
Expand Down Expand Up @@ -119,6 +122,8 @@ const InputProvider = ({ children, value: props }: Props) => {
setSearchTerm,
shouldAutocomplete,
toggleOpenButtonRef,
hideCaret,
setHideCaret,
};

return (
Expand Down
10 changes: 9 additions & 1 deletion @navikt/core/react/src/form/combobox/Input/Input.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -31,12 +31,15 @@ const Input = forwardRef<HTMLInputElement, InputProps>(
value,
searchTerm,
setValue,
hideCaret,
setHideCaret,
} = useInputContext();
const {
selectedOptions,
removeSelectedOption,
toggleOption,
isMultiSelect,
maxSelected,
} = useSelectedOptionsContext();
const {
activeDecendantId,
Expand Down Expand Up @@ -215,7 +218,10 @@ const Input = forwardRef<HTMLInputElement, InputProps>(
ref={mergedRefs}
value={value}
onBlur={() => virtualFocus.moveFocusToTop()}
onClick={() => value !== searchTerm && onChange(value)}
onClick={() => {
setHideCaret(!!maxSelected?.isLimitReached);
value !== searchTerm && onChange(value);
}}
onInput={onChangeHandler}
type="text"
role="combobox"
Expand All @@ -233,7 +239,9 @@ const Input = forwardRef<HTMLInputElement, InputProps>(
"navds-combobox__input",
"navds-body-short",
`navds-body-short--${size}`,
{ "navds-combobox__input--hide-caret": hideCaret },
)}
placeholder={!selectedOptions.length ? rest.placeholder : undefined}
/>
);
},
Expand Down
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import React, { forwardRef } from "react";
import { ChevronDownIcon, ChevronUpIcon } from "@navikt/aksel-icons";
import { useFilteredOptionsContext } from "../FilteredOptions/filteredOptionsContext";
import { useInputContext } from "./Input.context";

interface ToggleListButtonProps {
toggleListButtonLabel?: string;
Expand All @@ -11,10 +12,14 @@ export const ToggleListButton = forwardRef<
ToggleListButtonProps
>(({ toggleListButtonLabel }, ref) => {
const { isListOpen, toggleIsListOpen } = useFilteredOptionsContext();
const { focusInput } = useInputContext();
return (
<button
type="button"
onPointerUp={() => toggleIsListOpen()}
onPointerUp={() => {
toggleIsListOpen();
focusInput();
Copy link
Contributor Author

@JulianNymark JulianNymark Aug 6, 2024

Choose a reason for hiding this comment

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

I added focusInput() here because it seemed like a bug to not focus the input when you toggle the "show list of selections" via clicking on the button? 🤔 At least this seems like intended behavior now (post change).

Copy link
Collaborator

Choose a reason for hiding this comment

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

Might argue that its as intended for not focusing input. For desktopusers it might be nice to save a click, but for mobile/touch users a virtual keyboard will pop up when a textfield is focused. This might be worse then since if they wanted to type, they would have clicked the input.

Copy link
Contributor Author

@JulianNymark JulianNymark Aug 7, 2024

Choose a reason for hiding this comment

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

True, can argue for both. I did some testing just now however, and it seems like the behaviour is already like this on mobile? 🤔 (maybe I'm just not precise enough with my taps, and keep missing the dropdown button). If so it's not going to change anything on mobile.

I'd argue that since the arrow is visually "inside" the border rectangle.

image

Then this is an ok change? I'll remove it if you have a stronger opinion on it of course.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think I agree about this change. The only issue I see is that the focus is lost while the pointer is down, so that the focus border appears to blink.

I think the argument about the arrow being inside the border makes sense. Even though it might be annoying that the virtual keyboard pops up, you might not understand that you can write/search otherwise.

}}
onKeyDown={({ key }) => key === "Enter" && toggleIsListOpen()}
className="navds-combobox__button-toggle-list"
aria-expanded={isListOpen}
Expand Down
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import React, { useCallback, useMemo, useState } from "react";
import React, { useCallback, useEffect, useMemo, useState } from "react";
import { createContext } from "../../../util/create-context";
import { usePrevious } from "../../../util/hooks";
import { useInputContext } from "../Input/Input.context";
Expand Down Expand Up @@ -33,7 +33,7 @@ const SelectedOptionsProvider = ({
"allowNewValues" | "isMultiSelect" | "onToggleSelected" | "maxSelected"
> & { options: ComboboxOption[]; selectedOptions?: ComboboxOption[] };
}) => {
const { clearInput, focusInput } = useInputContext();
const { clearInput, focusInput, setHideCaret } = useInputContext();
const {
customOptions,
removeCustomOption,
Expand Down Expand Up @@ -101,6 +101,14 @@ const SelectedOptionsProvider = ({
[customOptions, onToggleSelected, removeCustomOption],
);

const isLimitReached =
(!!maxSelected?.limit && selectedOptions.length >= maxSelected.limit) ||
(!isMultiSelect && selectedOptions.length > 0);

useEffect(() => {
setHideCaret(isLimitReached);
}, [selectedOptions, setHideCaret, isLimitReached]);

const toggleOption = useCallback(
(
option: ComboboxOption,
Expand All @@ -125,9 +133,6 @@ const SelectedOptionsProvider = ({

const prevSelectedOptions = usePrevious<ComboboxOption[]>(selectedOptions);

const isLimitReached =
!!maxSelected?.limit && selectedOptions.length >= maxSelected.limit;

const selectedOptionsState = {
addSelectedOption,
isMultiSelect,
Expand Down
32 changes: 31 additions & 1 deletion @navikt/core/react/src/form/combobox/combobox.stories.tsx
Original file line number Diff line number Diff line change
@@ -1,9 +1,11 @@
import { Meta, StoryFn } from "@storybook/react";
import React, { useEffect, useMemo, useRef, useState } from "react";
import { Alert } from "../../alert";
import { Button } from "../../button";
import { Chips } from "../../chips";
import { VStack } from "../../layout/stack";
import { Modal } from "../../modal";
import { BodyLong } from "../../typography";
import { TextField } from "../textfield";
import { UNSAFE_Combobox } from "./index";

Expand Down Expand Up @@ -40,7 +42,7 @@ export const Default: StoryFunction = (props) => (

Default.args = {
options,
label: "Hva er dine favorittfrukter?",
label: "Hva er din favorittfrukt?",
shouldAutocomplete: true,
isLoading: false,
isMultiSelect: false,
Expand All @@ -67,6 +69,34 @@ Default.argTypes = {
},
};

export const WithPlaceholder: StoryFunction = () => {
const props = {
options,
label: "Hva er din favorittfrukt?",
shouldAutocomplete: true,
isLoading: false,
isMultiSelect: false,
allowNewValues: false,
onChange: console.log,
};
return (
<VStack gap="8" align="center">
<Alert variant="warning" style={{ width: "70ch" }}>
<VStack gap="4">
<BodyLong>
{`We don't endorse placeholders, but they shouldn't break either!`}
</BodyLong>
<BodyLong>
{`Don't
tell anyone that they work (or that they exist).`}
</BodyLong>
</VStack>
</Alert>
<UNSAFE_Combobox {...props} id="combobox" placeholder="placeholder" />
</VStack>
);
};

export const MultiSelect: StoryFn = () => {
const [selectedOptions, setSelectedOptions] = useState<string[]>([
"pear",
Expand Down
Loading