feat:extend-shortcuts #369
Conversation
|
Thank you for opening this PR! Before a maintainer takes a look, it would be really helpful if you could walk through your changes using GitHub's review tools. Please take a moment to:
More information on how to conduct a self review: This helps make the review process smoother and gives us a clearer understanding of your thought process. Once you've added your self-review, we'll continue from our side. Thank you! |
Neeraj-gagat
left a comment
Neeraj-gagat
left a comment
There was a problem hiding this comment.
added shortcuts keys to use in taskdialog
like up down arrow keys enter to go in edit mode
escape to exit edit mode when edit mode for one feild is open you can not open anot edit mode
and if you press escape when edit feild is not ope you can exit diaog but when edit feild is open you can exit edit feild
Neeraj-gagat
left a comment
Neeraj-gagat
left a comment
There was a problem hiding this comment.
done with the self review new chages break nothing all the test pass and it works fine
let me know if i need to make chnages
|
Hey @its-me-abhishek while working on this feat I tried to add shortcuts to calender so we can use arrow keys to navigate through dates and select them but it did not work out do you have any suggestions how can I do that ? |
its-me-abhishek
left a comment
its-me-abhishek
left a comment
There was a problem hiding this comment.
Also, do rename the PR title, old commits, as per the naming conventions in CONTRIBUTING.MD
| import { formattedDate, handleCopy } from './tasks-utils'; | ||
| import { useEffect, useRef, useState } from 'react'; | ||
|
|
||
| const FIELDS = [ |
There was a problem hiding this comment.
this should be moved to constants.ts file
| 'annotations', | ||
| ] as const; | ||
|
|
||
| type FieldKey = (typeof FIELDS)[number]; |
There was a problem hiding this comment.
similarly, move this to types.ts
| const focusedField = FIELDS[focusedFieldIndex]; | ||
|
|
||
| useEffect(() => { | ||
| const el = editButtonRef.current[focusedField]; |
There was a problem hiding this comment.
what does el suggest? should be renamed to a better variable name, which should be self explanatory
There was a problem hiding this comment.
this was shortform for element i think this is self explanatory if you want i can make it element let me know
There was a problem hiding this comment.
element would be better in this case
| }); | ||
| }, [focusedField]); | ||
|
|
||
| const focusMap: Record<string, () => void> = { |
There was a problem hiding this comment.
this map should be extracted out, as a task-utils function if possible. They will end up bloating the TaskDialog component finally. Making it harder to change
| target.tagName === 'INPUT' || | ||
| target.tagName === 'TEXTAREA' || | ||
| target.isContentEditable; | ||
| // ⛔ allow normal typing |
There was a problem hiding this comment.
Do remove these comments
| editState.isEditingAnnotations, | ||
| ]); | ||
|
|
||
| const handleDialogKeyDown = (e: React.KeyboardEvent) => { |
There was a problem hiding this comment.
maybe this can be abstracted out as well
| }; | ||
|
|
||
| const saveAndExit = (fn: () => void) => { | ||
| fn(); |
There was a problem hiding this comment.
fn shall be renamed as well
| }); | ||
| }; | ||
|
|
||
| const handledropdown = (type: FieldKey) => { |
There was a problem hiding this comment.
similar, should be moved to a utils file
| t.uuid | ||
| )} | ||
| readOnly | ||
| tabIndex={-1} |
There was a problem hiding this comment.
it was to prevent default focus but we dont actually need it will remove it in new chnages
On it 👍 |
its-me-abhishek
left a comment
its-me-abhishek
left a comment
There was a problem hiding this comment.
Please also add unit tests for all these new files/changes
| inputRefs, | ||
| }: UseTaskDialogFocusMapProps<F>) { | ||
| return React.useCallback( | ||
| (field: F[number]) => { |
There was a problem hiding this comment.
please revert to verbose variable names here as well. they usually lose context and become hard to guess over the time
There was a problem hiding this comment.
Sorry I did not get it what do I exactly need to do add type for fields or something else
There was a problem hiding this comment.
@its-me-abhishek can you please clear my doubt
There was a problem hiding this comment.
hi, i just meant, change this (and all other vars) to better names as you'd done here earlier
There was a problem hiding this comment.
ahh okkokk my bad changing it now
| } | ||
|
|
||
| export interface UseTaskDialogKeyboardProps<F extends readonly string[]> { | ||
| fields: F; |
| isOverdue, | ||
| isUnsynced, | ||
| }: EditTaskDialogProps) => { | ||
| const editButtonRef = useRef<Record<FieldKey, HTMLButtonElement | null>>( |
There was a problem hiding this comment.
if possible, replace all any to a data type. for typesafe code and easier tests
|
Hey @its-me-abhishek can you please clear my. doubt. About reverting changes ?? |
2 participants
Description
This Pr adds shortcuts for taskdialog
now we can naigate through task feilds using up && down arrow keys and
and by enter you can edit the feild
by escape yo can exit the editing state or by pressing escape when you are not editing anything you can exit dialog
- Fixes: #319
Additional Notes
will raise other pr for addtask dialog
video
Screencast.from.04-01-26.08.01.11.AM.IST.webm
let me know if i need to do any changes