Adding keyboard shortcuts to common actions by Adaverse · Pull Request #30950 · apache/airflow

Adaverse · 2023-04-28T20:41:13Z

closes: #22870

Hello,

Added below shortcuts to the DAGS grid screen -

At DagRun
- Clear existing tasks (shift + c)
- Mark as success or fail (shift + s or shift + f)
- Toggle note (shift + n)
- Edit notes (shift + e)
At TaskInstance
- Clear task (shift + c)
- Mark as success or fail (shift + s or shift + f)
- Toggle notes (shift + n)
- Edit notes (shift + e)
Toggle shortcut cheat sheet (shift + /)

Added the shortcut cheat sheet modal -

The tasks

Implementing various shortcuts for grid view
Creating a cheat sheet for the information
keeping focus on the action buttons of the opened modal by default

^ Add meaningful description above

Read the Pull Request Guidelines for more information.
In case of fundamental code changes, an Airflow Improvement Proposal (AIP) is needed.
In case of a new dependency, check compliance with the ASF 3rd Party License Policy.
In case of backwards incompatible changes please leave a note in a newsfragment file, named {pr_number}.significant.rst or {issue_number}.significant.rst, in newsfragments.

eladkal · 2023-04-28T21:18:34Z

This is cool!
One thing comes to mind is how users will be aware of the keyboard shortcuts?
I think we may need hints or/and each page/tab contains a menu explaining page shorcuts avaliable?

Adaverse · 2023-04-28T21:39:34Z

This is cool! One thing comes to mind is how users will be aware of the keyboard shortcuts? I think we may need hints or/and each page/tab contains a menu explaining page shorcuts avaliable?

Yes! Like YouTube, we can have a modal (at each tab level) that popups upon clicking a button that will show all the shortcuts relevant to that tab. On YouTube, we can see that by hitting shift+/. Let me compile all the shortcuts and list them down. If you guys have anything, do add it. Just want to get this code reviewed before adding the rest :).

bbovenzi · 2023-05-02T17:57:15Z


+  const initialClearButtonFocusRef = useRef<HTMLButtonElement>(null);
+
+  useKeysPress("shiftKey", ['c', 'C'], onOpen);


Cool! Do you have a list of what shortcuts you want to do? We'll need to check this across browsers or toggle on/off if it conflicts with a user's shortcuts for something else.

Hi @bbovenzi,

Added below shortcuts to the DAGS grid screen -

At DagRun

Clear existing tasks (shift + c)

Mark as success or fail (shift + s or shift + f)

Add note (shift + n)

At TaskInstance

Clear task (shift + c)

Mark as success or fail (shift + s or shift + f)

Add note (shift + n)

The shortcuts are the same, just depends on what is selected. Will add more in the process.

Yes, I have checked with major browsers (Chrome, Firefox, Edge, Opera), and these shortcuts aren't used as of yet (At least nothing happened when I clicked :)).

Hi @bbovenzi,
I have fixed all the issues. Can you have a look once at the implementation?

Looking good!
Do we want to add a cheatsheet somewhere?
Also, for adding a note, we should make sure to toggle the accordion open if it is closed.

Thanks for having a look.

Yes, working on creating the cheat sheet.

Yes, will make sure to toggle the notes accordion open (if closed).

Looking good! Do we want to add a cheatsheet somewhere? Also, for adding a note, we should make sure to toggle the accordion open if it is closed.

Hi @bbovenzi

Added a cheat sheet for shortcuts

Yes, toggling the accordion and making the text area in focus

…list - At DagRun - Clear existing tasks (shift + c) - Mark as success or fail (shift + s or shift + f) - Add note (shift + n) - At TaskInstance - Clear task (shift + c) - Mark as success or fail (shift + s or shift + f) - Add note (shift + n)

…cut cheat sheet - added cheat sheet modal - using local storage variable to ignore event which text area is in focus

Adaverse · 2023-05-15T21:09:04Z

This is cool! One thing comes to mind is how users will be aware of the keyboard shortcuts? I think we may need hints or/and each page/tab contains a menu explaining page shorcuts avaliable?

Hi @eladkal,
Added a modal listing all the shortcuts accessible using the shortcut (shitf+/). I need some suggestions on where to put this text (or hint) to let the user know, like "Press Shift + / to view all shortcuts" at the same time making sure that it does not look out of place. The other place is in the Airflow documentation on DAG and Task run.

Adaverse · 2023-05-18T16:32:03Z

Hi @bbovenzi ,

Fixed the accordion toggle logic making it generic
Using the local storage key "isInputInFocus" to discard the events that would get fired when the user is typing. Tried methods like e.stopPropagation(), e.native.stopImmediatePropagation(). Also tried this patch from react - https://legacy.reactjs.org/blog/2020/08/10/react-v17-rc.html#changes-to-event-delegation but still no luck. Finally used the above-mentioned tactical solution when on focus, setting it to true, and on blur setting it to false.

Do let me know if I missed anything.

…list - At DagRun - Clear existing tasks (shift + c) - Mark as success or fail (shift + s or shift + f) - Add note (shift + n) - At TaskInstance - Clear task (shift + c) - Mark as success or fail (shift + s or shift + f) - Add note (shift + n)

…cut cheat sheet - added cheat sheet modal - using local storage variable to ignore event which text area is in focus

Adaverse · 2023-05-22T16:22:44Z

Hi @bbovenzi ,

Fixed the accordion toggle logic making it generic

Using the local storage key "isInputInFocus" to discard the events that would get fired when the user is typing. Tried methods like e.stopPropagation(), e.native.stopImmediatePropagation(). Also tried this patch from react - https://legacy.reactjs.org/blog/2020/08/10/react-v17-rc.html#changes-to-event-delegation but still no luck. Finally used the above-mentioned tactical solution when on focus, setting it to true, and on blur setting it to false.

Do let me know if I missed anything.

@bbovenzi any thoughts on the above?

bbovenzi

Left a few comments.

bbovenzi · 2023-05-24T19:24:09Z

  cache: EmotionCache;
 }

+const isInputInFocus: IsInputInFocus = "isInputInFocus";


it's just a string we don't need a special type for it.

Suggested change

const isInputInFocus: IsInputInFocus = "isInputInFocus";

const isInputInFocus = "isInputInFocus";

Here the idea behind having a type for a string was to enforce this to have only specific values (since it is the key of the local storage object). In the above case, it was below.

// In type/index.ts type IsInputInFocus = "isInputInFocus";

We can replace it with a global constant that will be imported wherever used. Where can I define a constant to import it anywhere required? I do not seem to find where we define all constants used in UI.

Just exporting the string from the useKeysPress is fine.

Done. Defined in useKeysPress and importing it everywhere.

bbovenzi · 2023-05-24T19:31:32Z

  };

+  useKeysPress(
+    keyboardShortcutIdentifier.toggleShortcutCheatSheet.primaryKey,


Could we just pass keyboardShortcutIdentifier.toggleShortcutCheatSheet and onToggleShortcut?

This is done

bbovenzi · 2023-05-24T19:33:58Z

+    keyboardShortcutIdentifier.viewNotes.secondaryKey,
+    () => {
+      // toggling notes accordion (index = 0)
+      toggleNotesAccordionIthIndex(0);


I don't think we need all this extra toggleNotesAccordionIthIndex just toggle the editMode

True! But implemented toggleNotesAccordionIthIndex, thinking new panels might be included in the accordion in the future. Since you think that it might be overkill, hence replaced it with the simplest implementation - toggleNotesPanel

bbovenzi · 2023-05-24T19:36:02Z

+  },
+};
+
+export default keyboardShortcutIdentifier;


Should we put a small prompt somewhere so users even know this exists?

Added a button. Let me know your thoughts

Text could be smaller, and we could just say : "Type shift+/ for shortcuts

- fixed static check failure

- added isInputInFocus in useKeysPress and importing it from here everywhere

bbovenzi

looking good!

Adaverse · 2023-05-29T16:36:57Z

@bbovenzi can this be merged now?

useKeysPress

78cb051

Adaverse requested review from ashb, bbovenzi and ryanahamilton as code owners April 28, 2023 20:41

boring-cyborg Bot added area:UI Related to UI/UX. For Frontend Developers. area:webserver Webserver related Issues labels Apr 28, 2023

Adaverse changed the title # Adding keyboard shortcuts to common actions Apr 28, 2023

Adaverse marked this pull request as draft April 29, 2023 16:21

bbovenzi reviewed May 2, 2023

View reviewed changes

Adaverse requested a review from bbovenzi May 3, 2023 11:13

Adaverse and others added 3 commits May 8, 2023 22:59

static checks resolved

63f3741

Merge branch 'main' into test

0be7724

- added shortcut for edit notes, toggle notes accordion, toggle short…

500ae1e

…cut cheat sheet - added cheat sheet modal - using local storage variable to ignore event which text area is in focus

Adaverse marked this pull request as ready for review May 15, 2023 21:01

Adaverse added 4 commits May 16, 2023 10:04

fixed static checks

a6cbf66

fixed notes accordion toggle logic

43a5246

fixed test

a8f64d1

static checks fixed

d639424

Adaverse added 7 commits May 18, 2023 22:39

useKeysPress

a8f5f8b

static checks resolved

7e609dc

- added shortcut for edit notes, toggle notes accordion, toggle short…

c06003c

…cut cheat sheet - added cheat sheet modal - using local storage variable to ignore event which text area is in focus

fixed static checks

88add77

fixed notes accordion toggle logic

7707d39

fixed test

4807d1c

static checks fixed

275d4a2

Adaverse force-pushed the test branch from d639424 to 275d4a2 Compare May 18, 2023 17:09

eladkal requested a review from pierrejeambrun May 23, 2023 05:18

bbovenzi requested changes May 24, 2023

View reviewed changes

bbovenzi mentioned this pull request May 25, 2023

Navigate grid view via keyboard #31556

Closed

2 tasks

Adaverse added 4 commits May 26, 2023 00:20

Code reivew changes - reduced useKeysPress function signature

5a4bf5c

Code review changes - reduced funtion signature of usekeysPress

7850e30

- removed toggleNotesAccordionIthIndex with more simple toggleNotesPanel

cc0f16f

- fixed static check failure

- Removed type defined a string type

ddd1dd5

- added isInputInFocus in useKeysPress and importing it from here everywhere

Adaverse requested a review from bbovenzi May 26, 2023 12:07

added prompt for shortcut

02934c0

bbovenzi approved these changes May 26, 2023

View reviewed changes

Merge branch 'main' into test

221fc83

bbovenzi merged commit f532d39 into apache:main May 29, 2023

bbovenzi added this to the Airflow 2.7.0 milestone May 29, 2023

eladkal added the type:new-feature Changelog: New Features label Jun 8, 2023


		const initialClearButtonFocusRef = useRef<HTMLButtonElement>(null);

		useKeysPress("shiftKey", ['c', 'C'], onOpen);

	const isInputInFocus: IsInputInFocus = "isInputInFocus";
	const isInputInFocus = "isInputInFocus";

Uh oh!

Conversation

Adaverse commented Apr 28, 2023 • edited by eladkal Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

eladkal commented Apr 28, 2023

Uh oh!

Adaverse commented Apr 28, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bbovenzi May 10, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Adaverse May 13, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Adaverse commented May 15, 2023

Uh oh!

Adaverse commented May 18, 2023

Uh oh!

Adaverse commented May 22, 2023

Uh oh!

bbovenzi left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Adaverse May 25, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bbovenzi left a comment

Choose a reason for hiding this comment

Uh oh!

Adaverse commented May 29, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Adaverse commented Apr 28, 2023 •

edited by eladkal

Loading

Adaverse commented Apr 28, 2023 •

edited

Loading

bbovenzi May 10, 2023 •

edited

Loading

Adaverse May 13, 2023 •

edited

Loading

bbovenzi left a comment •

edited

Loading

Adaverse May 25, 2023 •

edited

Loading