Add input focus with keyboard #896

LeWawan · 2021-10-29T08:16:30Z

Description

🚀 Add ⌘ + k && Ctrl + k to focus the input element. See #894

Tests

All tests passed.

vercel · 2021-10-29T08:16:35Z

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/carloscuesta/gitmoji/5DWSyNu3VwTRtJAu3dUwdbJX3fLo
✅ Preview: https://gitmoji-git-fork-lewawan-add-input-focus-wi-36e459-carloscuesta.vercel.app

src/components/GitmojiList/Toolbar/index.js

src/components/GitmojiList/Toolbar/styles.module.css

vhoyer

I tested this, and in my machine it doesn't work on chrome, as chrome focuses on the URL Bar when ctrl-k, maybe you forgot a preventDefault?

src/components/GitmojiList/Toolbar/index.js

src/components/GitmojiList/Toolbar/styles.module.css

📸 Update snapshots

vhoyer · 2021-10-29T18:42:55Z

I've also created this PR onto your branch to help you with the CSS changes, you can also propose suggestions to the changes I made there:
LeWawan#1

best

LeWawan · 2021-10-29T20:04:06Z

Thanks for all your feedback @vhoyer and @carloscuesta ! I'm coming from vuejs, so I'm not too familiar with best practices in React but I'll do my best to improve the PR!

vhoyer · 2021-10-29T20:57:22Z

Oh, no problem, I also come from vuejs so I know the struggle haha

…stions 💄 Prevent magic numbers and shift in layout ♿ Choose better colors for accessibility/contrast

LeWawan · 2021-10-30T00:26:43Z

Hey @carloscuesta

I've updated the PR 🚀

Thanks to @vhoyer for the help 🤘

Things to change:

conditionally render CMD for macOS and CTRL for Windows/Linux systems
use useEffect to remove the event when the component unmount
use useRef to focus the input
update style magic numbers #1 (@vhoyer)
ctrl-k focus the browser navigation bar (reported by @vhoyer)

P.S @vhoyer i've update the position of the keyboard indicator in the input, update the colors and check with WCAG.

If you want to discuss of the position of the indicator and/or the colors, i'm open.

carloscuesta · 2021-10-30T17:23:40Z

src/components/GitmojiList/Toolbar/index.js

-    />
-  </div>
-)
+const searchInputRef = createRef()


I would try to use useRef instead 😊

https://reactjs.org/docs/hooks-reference.html#useref

carloscuesta · 2021-10-30T17:24:06Z

src/components/GitmojiList/Toolbar/index.js

+    return () => {
+      document.removeEventListener('keydown', keyboardEventListener, false)
+    }
+  })


Suggested change

})

}, [])

src/components/GitmojiList/Toolbar/styles.module.css

carloscuesta · 2021-10-30T17:26:46Z

The margin that kbd has, is causing a little issue when the input is focused:

…cus-with-keyboard

LeWawan · 2021-10-31T16:06:43Z

Hey @carloscuesta

I've updated the PR 🚀

Things to change:

Add empty dependency array to useEffect
Update the css to prevent weird focus margin

The margin that kbd has, is causing a little issue when the input is focused:

@carloscuesta I can't reproduce the bug in my environment (Mac & Windows) but I have tweaked the css and I think it should fix the issue.

carloscuesta

Thanks! Looking much better now, found another issue with the alignment of the kbd indicator:

I'm using Safari Version 15.0 (16612.1.29.41.4, 16612)

If you can't reproduce it let me know so I can take a look

Thanks for your work! 😊

LeWawan · 2021-10-31T23:51:11Z

I think this time it's the right one 🚀

I tested on the same version as you : Version 15.0 (16612.1.29.41.4, 16612)

Thank you for let me do the feature 😁

vhoyer · 2021-11-01T01:52:20Z

src/components/GitmojiList/Toolbar/styles.module.css

+  align-items: center;
+  border-radius: 3px;
+  border: solid 1px #999;
+  color: #b8b8b8;


P.S @vhoyer i've update the position of the keyboard indicator in the input, update the colors and check with WCAG.

I've just checked it out and it seems to fail the tests 🤔

Oh boy, I tested it with the dark theme 😅

I update the code to put back the value you put for the light theme (#595959) and I put (#b8b8b8) for the dark theme

vhoyer · 2021-11-01T01:56:32Z

do you see the problem with using the indicator above the input, that's why I left if beside the input, if the problem is the outline around the "input/inputWrapper", we could use a custom :focus-within rule to add the outline on the correct visual element.

vhoyer · 2021-11-01T01:57:56Z

src/components/GitmojiList/Toolbar/styles.module.css

+  color: #b8b8b8;
+  display: flex;
+  font-family: system-ui;
+  margin: 0 0.5rem;


Suggested change

margin: 0 0.5rem;

margin-right: 0.5rem;

It's a good practice to never set properties you are not using with CSS

LeWawan · 2021-11-01T11:33:18Z

do you see the problem with using the indicator above the input, that's why I left if beside the input, if the problem is the outline around the "input/inputWrapper", we could use a custom :focus-within rule to add the outline on the correct visual element.

@vhoyer I totally agree with the :focus-within. I didn't like the indicator outside the input because i think that it's clearer for the user if it's inside the input.

…tmoji into add-input-focus-with-keyboard

vhoyer · 2021-11-01T11:45:54Z

src/components/GitmojiList/Toolbar/index.js

+  const keyboardEventListener = (event: KeyboardEvent) => {
+    const searchInput = searchInputRef.current
+    if (searchInput && (event.ctrlKey || event.metaKey) && event.key === 'k') {
+      event.preventDefault()
+      searchInput.focus()
+    }
+  }


It's best you create this function inside the useEffect, you can read more about it here: https://overreacted.io/a-complete-guide-to-useeffect/

I'll check it out, thanks for the tip 😄

Okay. got it ! Each render has it's own "instance" so it's better to create the function inside. However in this specific situation, it's not required for the right functioning to do it but rather to follow good practices right ?

correct, good practice prevents future errors

vhoyer · 2021-11-01T11:49:45Z

src/components/GitmojiList/Toolbar/styles.module.css

+}
+
+.inputWrapper:focus-within {
+  outline: black auto 1px;


I haven't tested this as I'm in mobile right now, but, isn't there an "initial" value to be used here?, I'm worried about this black on the dark theme

I've foud this value: -webkit-focus-ring-color but i wonder if it's compatible with all the browsers

probably not

carloscuesta · 2021-11-01T18:26:53Z

src/components/GitmojiList/Toolbar/styles.module.css

+.searchInput:focus-visible {
+  outline: none;
+}
+
+.searchInput:focus {
+  outline: none;
+}


I think we should keep this for a11y

I'm not sure but i think the focus-within keep the accessibility okay

It's not a problem to remove the outline if we provide an suitable replacement for it :D

carloscuesta · 2021-11-01T18:27:12Z

src/components/GitmojiList/Toolbar/styles.module.css

+  right: 0;
+  align-items: center;
+  border-radius: 3px;
+  border: solid 1px #999;


Maybe we should move this color into a CSS variable to support both light and dark themes?

It already support light and dark theme actually, i've tested it with a WCAG tool to make it usable for both

carloscuesta · 2021-11-01T18:27:21Z

src/components/GitmojiList/Toolbar/styles.module.css

+  align-items: center;
+  border-radius: 3px;
+  border: solid 1px #999;
+  color: #595959;


Same! CSS variable?

carloscuesta · 2021-11-01T18:27:48Z

src/components/GitmojiList/Toolbar/styles.module.css

+.inputWrapper:focus-within {
+  outline: -webkit-focus-ring-color auto 1px;
+}


Why do we need to specify this?

This one is the replacement for the focus && focus-visible: none

carloscuesta

🚀

Thanks for the work and time you put into this ❤️

LeWawan added 4 commits October 27, 2021 17:33

✨ Add keyboard event handler to focus on searchInput

7a8f6fd

💄 Update searchInput to add keyboard kbd

f7196c6

📸 Update shapshots for gitmojiList & page

69a6e38

✅ Fix flow errors

268bc80

vercel bot deployed to Preview October 29, 2021 08:16 View deployment

Merge branch 'master' into add-input-focus-with-keyboard

c9a4da8

vercel bot deployed to Preview October 29, 2021 08:18 View deployment

carloscuesta reviewed Oct 29, 2021

View reviewed changes

vhoyer requested changes Oct 29, 2021

View reviewed changes

vhoyer added 2 commits October 29, 2021 15:37

💄 Prevent magic numbers and shift in layout

073b529

📸 Update snapshots

♿ Choose better colors for accessibility/contrast

8f6a60b

Merge pull request #1 from vhoyer/add-input-focus-with-keyboard-sugge…

2d5af44

…stions 💄 Prevent magic numbers and shift in layout ♿ Choose better colors for accessibility/contrast

vercel bot deployed to Preview October 29, 2021 21:30 View deployment

LeWawan added 4 commits October 30, 2021 01:18

🎨 Add useRef to remove event

bb7bc4d

💄 Tweak keyboard hint position

1ca62de

✅ Add jest spy on navigator.platform to ensure right returns

e848859

📸 Update Snapshots

34159df

vercel bot deployed to Preview October 29, 2021 23:49 View deployment

LeWawan added 3 commits October 30, 2021 02:06

🍻 Fix isMacOs function

f49f351

✅ Remove jest spy (window is undefined in the wrapper)

98547d8

📸 Fix snapshots

0fe4f86

vercel bot deployed to Preview October 30, 2021 00:09 View deployment

carloscuesta reviewed Oct 30, 2021

View reviewed changes

carloscuesta linked an issue Oct 31, 2021 that may be closed by this pull request

⌘ + k && Ctrl + k to focus the input element #894

Closed

LeWawan added 3 commits October 31, 2021 16:38

Merge branch 'master' of github.com:LeWawan/gitmoji into add-input-fo…

8051789

…cus-with-keyboard

💄 Tweak css to prevent weird focus margin

4f4ec53

🔥 Use "useRef" instead of "createRef"

3b6cd3a

vercel bot deployed to Preview October 31, 2021 16:01 View deployment

carloscuesta reviewed Oct 31, 2021

View reviewed changes

💄 Fix kbd alignment on safari

3fd497b

vercel bot deployed to Preview October 31, 2021 23:40 View deployment

vhoyer reviewed Nov 1, 2021

View reviewed changes

♿️ Fix accessibility contrast issue for light theme

819f90a

LeWawan added 2 commits November 1, 2021 12:34

💄 Add focus-within to the inputWrapper

c658e40

Merge branch 'add-input-focus-with-keyboard' of github.com:LeWawan/gi…

69b1307

…tmoji into add-input-focus-with-keyboard

vercel bot deployed to Preview November 1, 2021 11:37 View deployment

vhoyer reviewed Nov 1, 2021

View reviewed changes

LeWawan added 2 commits November 1, 2021 13:11

🎨 create keyboardEventListener inside useEffet

5753e97

🐛 Update focus-within from black to -webkit-focus-ring-color

36fe939

vercel bot deployed to Preview November 1, 2021 12:13 View deployment

🐛 Fix focus on safari

74a357b

vercel bot deployed to Preview November 1, 2021 12:16 View deployment

carloscuesta reviewed Nov 1, 2021

View reviewed changes

carloscuesta approved these changes Nov 3, 2021

View reviewed changes

carloscuesta merged commit 134ba89 into carloscuesta:master Nov 3, 2021

github-actions bot locked as resolved and limited conversation to collaborators Nov 5, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add input focus with keyboard #896

Add input focus with keyboard #896

LeWawan commented Oct 29, 2021 •

edited

vercel bot commented Oct 29, 2021 •

edited

vhoyer left a comment

vhoyer commented Oct 29, 2021

LeWawan commented Oct 29, 2021

vhoyer commented Oct 29, 2021

LeWawan commented Oct 30, 2021

carloscuesta Oct 30, 2021

carloscuesta Oct 30, 2021

carloscuesta commented Oct 30, 2021

LeWawan commented Oct 31, 2021

carloscuesta left a comment •

edited

LeWawan commented Oct 31, 2021

vhoyer Nov 1, 2021

LeWawan Nov 1, 2021

vhoyer commented Nov 1, 2021

vhoyer Nov 1, 2021

LeWawan commented Nov 1, 2021

vhoyer Nov 1, 2021

LeWawan Nov 1, 2021

LeWawan Nov 1, 2021

vhoyer Nov 5, 2021

vhoyer Nov 1, 2021

LeWawan Nov 1, 2021

vhoyer Nov 5, 2021

carloscuesta Nov 1, 2021

LeWawan Nov 3, 2021

vhoyer Nov 5, 2021

carloscuesta Nov 1, 2021

LeWawan Nov 3, 2021

carloscuesta Nov 1, 2021

carloscuesta Nov 1, 2021

LeWawan Nov 3, 2021

carloscuesta left a comment

Add input focus with keyboard #896

Add input focus with keyboard #896

Conversation

LeWawan commented Oct 29, 2021 • edited

Description

Tests

vercel bot commented Oct 29, 2021 • edited

vhoyer left a comment

Choose a reason for hiding this comment

vhoyer commented Oct 29, 2021

LeWawan commented Oct 29, 2021

vhoyer commented Oct 29, 2021

LeWawan commented Oct 30, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

carloscuesta commented Oct 30, 2021

LeWawan commented Oct 31, 2021

carloscuesta left a comment • edited

Choose a reason for hiding this comment

LeWawan commented Oct 31, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vhoyer commented Nov 1, 2021

Choose a reason for hiding this comment

LeWawan commented Nov 1, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

carloscuesta left a comment

Choose a reason for hiding this comment

LeWawan commented Oct 29, 2021 •

edited

vercel bot commented Oct 29, 2021 •

edited

carloscuesta left a comment •

edited