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
feat: Add a keyboard shortcut to move focus to search #4796
Conversation
75d7d26
to
6e4d8ce
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like this, although /
is bound to a useful Firefox shortcut as well (quick find in page). In this case, overriding the FF shortcut would add value for me tho.
IMHO, it would be nice to have more keyboard shortcuts for the UI, but I feel we should generally think about the shortcuts and their integration a little, before randomly introducing them, ensuring a consistent user experience (also across browsers).
@@ -40,6 +40,16 @@ const APP_FIELDS = [ | |||
const APP_LIST_FIELDS = ['metadata.resourceVersion', ...APP_FIELDS.map(field => `items.${field}`)]; | |||
const APP_WATCH_FIELDS = ['result.type', ...APP_FIELDS.map(field => `result.application.${field}`)]; | |||
|
|||
document.onkeypress = e => { | |||
if (e.keyCode === 47) { | |||
const searchBar = document.getElementById('search-bar'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as #4801 (comment) . Please use React.useRef instead of document.getElementById('search-bar')
@@ -40,6 +40,16 @@ const APP_FIELDS = [ | |||
const APP_LIST_FIELDS = ['metadata.resourceVersion', ...APP_FIELDS.map(field => `items.${field}`)]; | |||
const APP_WATCH_FIELDS = ['result.type', ...APP_FIELDS.map(field => `result.application.${field}`)]; | |||
|
|||
document.onkeypress = e => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overriding onkeypress might be unpleasant if we add a key press handler in another component.
Please use useEffect hook to add handler with document.addEventListener
and remove listener in hook cleanup. E.g.
useEffect(() => {
const handleKeyPress = (e) => {
...
}
document.addEventListener('keypress', handleKeyPress);
return () => {
document.removeEventListener('keypress', handleKeyPress);
};
});
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've updated the code to use React.useEffect
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We use /
while adding the git repository in the application create form. We shouldn't consider /
as a special shortcut when the Application form is opened.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@alexmt I've added a check to disable this shortcut when the new application form is opened.
8ba161c
to
b5344fa
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
@chetan-rns , can you please resolve merge conflicts? |
Fixes: argoproj#4700 Signed-off-by: Chetan Banavikalmutt <chetanrns1997@gmail.com>
b5344fa
to
309fa17
Compare
Codecov Report
@@ Coverage Diff @@
## master #4796 +/- ##
=======================================
Coverage 41.13% 41.13%
=======================================
Files 129 129
Lines 17875 17875
=======================================
Hits 7353 7353
Misses 9459 9459
Partials 1063 1063 Continue to review full report at Codecov.
|
Fixes: #4700 Signed-off-by: Chetan Banavikalmutt <chetanrns1997@gmail.com> Signed-off-by: Remington Breeze <remington@breeze.software>
Pressing "/" will move the focus to the search bar on the applications page
Fixes: #4700
Signed-off-by: Chetan Banavikalmutt chetanrns1997@gmail.com
Checklist: