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

Conversation

JulianNymark
Copy link
Contributor

@JulianNymark JulianNymark commented Aug 6, 2024

combobox hide caret on selection (mouse or keyboard)

closes #2891

Hides the blinking cursor/caret when it's appropriate (on isLimitReached for multiSelect, and also on selection for single select). Should help indicate that "there is nothing more to do here" in the combobox.

closes https://github.com/navikt/team-aksel/issues/568

Fixes the placeholder bug, so placeholders will disappear when an option is selected.

ticks box in https://github.com/navikt/team-aksel/issues/524: Når bruker velger noe med "Enter" slutter "input-cursor å blinke (frem til de begynner å skrive på nytt)

Copy link

changeset-bot bot commented Aug 6, 2024

🦋 Changeset detected

Latest commit: 4702bfc

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 7 packages
Name Type
@navikt/ds-react Patch
@navikt/ds-css Patch
@navikt/aksel-stylelint Patch
@navikt/aksel Patch
@navikt/ds-tokens Patch
@navikt/ds-tailwind Patch
@navikt/aksel-icons Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@@ -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 [showCaret, setShowCaret] = useState(true);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

ideally I wanted to solve this with pure CSS, but on some deliberation, I found it to require a state / state-machine.

"when you open the list of suggestions / focus the input show blinking caret, but don't show it even when focused if the user recently selected something"

Copy link
Contributor

github-actions bot commented Aug 6, 2024

Storybook demo

Endringer til review: 4

30f91d5ae | 88 komponenter | 139 stories

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.

@JulianNymark JulianNymark changed the title 🚸 hide caret on select (until list opened) 🚸 hide caret on select (until options list opened) Aug 6, 2024
@it-vegard
Copy link
Contributor

Should the caret be hidden in multiselect Comboboxes? (It is now) For these, we can still select more values, so maybe we should indicate this by showing the caret? We could possibly hide it if the maximum selected options is reached. 🤔

Also, clicking the input field should probably show the caret again. Right now the user has to click outside the Combobox or open the list to bring back the caret. (Or start typing, which is a hidden feature)

@JulianNymark
Copy link
Contributor Author

JulianNymark commented Aug 6, 2024

Also, clicking the input field should probably show the caret again. Right now the user has to click outside the Combobox or open the list to bring back the caret. (Or start typing, which is a hidden feature)

yeah... a bit unfortunate, I think it's because it's technically focused, but not virtually focused? 🤔

@JulianNymark
Copy link
Contributor Author

Also, clicking the input field should probably show the caret again. Right now the user has to click outside the Combobox or open the list to bring back the caret. (Or start typing, which is a hidden feature)

Should be fixed now (with more edge-case code logic 🙈 )

@JulianNymark
Copy link
Contributor Author

I also fixed the broken placeholder in this branch (seems like overkill to make a whole branch for that, it has its own internal story too, don't want to talk loudly about placeholders 🙊 )

@KenAJoh
Copy link
Collaborator

KenAJoh commented Aug 7, 2024

Would it make sense to hide the caret when in multiselect mode and isLimitReached?

@JulianNymark
Copy link
Contributor Author

Would it make sense to hide the caret when in multiselect mode and isLimitReached?

Yeah, @it-vegard also suggested that above (i'm still struggling to bring the isLimitReached state change into the useCallback hook 😳, trying different things with useRef, but it seems to break things...), at least it's now always on for multiselect, which is a step better. Getting the caret to be hidden on reaching the limit.

@JulianNymark
Copy link
Contributor Author

it seems to be perfect now! (fixes all the mentioned issues) 🤞

@HalvorHaugan
Copy link
Contributor

Remember changeset ;)

@HalvorHaugan HalvorHaugan changed the title 🚸 hide caret on select (until options list opened) Combobox: 🚸 hide caret on select (until options list opened) Aug 26, 2024
@JulianNymark JulianNymark merged commit c98ab95 into main Sep 2, 2024
4 checks passed
@JulianNymark JulianNymark deleted the combobox-hide-caret-on-select branch September 2, 2024 10:14
@github-actions github-actions bot mentioned this pull request Sep 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Innspill Combobox] Ser ut som at man skal velge flere ting i single select: <Combobox />
4 participants