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

feat: Add keyboard shortcuts to pause, go to next test, and toggle scrolling in e2e Test Runner #16411

Merged
merged 29 commits into from
May 20, 2021
Merged

Conversation

Manuel-Suarez-Abascal
Copy link
Contributor

@Manuel-Suarez-Abascal Manuel-Suarez-Abascal commented May 10, 2021

User facing changelog

Additional details

Added keyboard events to control the test runner:

  • Press the c key to continue a paused test runner.
  • Press the n key to go to the next test with a paused test runner.
  • Press the a key to toggle scrolling.

Also, added modal with shortcuts help documentation for users. Finally, included the required tests for the new features & documentation.

Note: I didn't implement the p key to pinned the test runner. I'm not exactly sure how to tackle it yet. I would like to discuss this implementation & open a new PR for it later on. Thoughts?

How has the user experience changed?

Shortcut help modal:
https://user-images.githubusercontent.com/32891808/117596749-0963df80-b112-11eb-8ac1-26abf33d44fd.mov

Toggle scrolling with a key:
https://user-images.githubusercontent.com/32891808/117596966-90b15300-b112-11eb-97ab-f7139d1ec608.mov

Next test on paused test runner with n key:
https://user-images.githubusercontent.com/32891808/117597060-c5bda580-b112-11eb-9911-a1eaf00e926c.mov

Continue test on paused test runner with c key:
https://user-images.githubusercontent.com/32891808/117597159-0a494100-b113-11eb-938b-bb499d5b293b.mov

PR Tasks

  • Have tests been added/updated?
  • Added keyboard events to control test runner.
  • Added modal with shortcuts help documentation.

@Manuel-Suarez-Abascal Manuel-Suarez-Abascal requested a review from a team as a code owner May 10, 2021 01:59
@Manuel-Suarez-Abascal Manuel-Suarez-Abascal requested review from flotwig and chrisbreiding and removed request for a team May 10, 2021 01:59
@cypress-bot
Copy link
Contributor

cypress-bot bot commented May 10, 2021

Thanks for taking the time to open a PR!

@Manuel-Suarez-Abascal Manuel-Suarez-Abascal changed the title Issue 248 Issue-248 May 10, 2021
@jennifer-shehane jennifer-shehane requested review from jennifer-shehane and removed request for flotwig and chrisbreiding May 10, 2021 13:43
@jennifer-shehane jennifer-shehane changed the title Issue-248 feat: Add keyboard shortcuts to pause, go to next test, and toggle scrolling in e2e Test Runner May 10, 2021
Copy link
Member

@jennifer-shehane jennifer-shehane left a comment

Choose a reason for hiding this comment

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

Thanks for all the work!

  1. We like to see more in-product documentation of features, like the shortcut modal. But I don't think this modal is in quite the right place. Since the shortcuts are only relevant after clicking inside a test, this would be better in the Runner (although I know space is limited in the UI now). Also, we will likely be unifying the component testing within the Desktop-GUI which would further confuse the keyboard shortcuts. Maybe we should just remove the modal in place of Integration with Travis CI #2?
  2. I'd like to see the new shortcuts documented within the tooltip of the action, similar to how the other actions show.
    Screen Shot 2021-05-13 at 11 43 35 AM
    tcut tooltips show the keyboard shortcut.
    Screen Shot 2021-05-13 at 11 43 30 AM
  3. I wouldn't worry about the pinning shortcut.

Larger discussion points

We discussed some things in our team meeting that are larger discussion points for these keyboard shortcuts in general - not specific to this PR. How this PR was implemennted is fine because it follows how we originally implemented the keyboard shortcuts.

  1. The keyboard shortcuts don't work if you have a test running that is typing into the AUT OR if you manually focus an input for typing into the AUT. This pulls focus away. You can see this by running a test that does a lot of typing and trying to toggle the 'auto-focus' over and over. It just stops working.
  2. We probably should have made these keyboard shortcuts multi-key. The single keys can cause confusion if you are playing around with the AUT, interacting with inputs, then want to 'rerun' your tests for example. It's just broken.

@Manuel-Suarez-Abascal
Copy link
Contributor Author

Manuel-Suarez-Abascal commented May 17, 2021

Thanks for all the work!

  1. We like to see more in-product documentation of features, like the shortcut modal. But I don't think this modal is in quite the right place. Since the shortcuts are only relevant after clicking inside a test, this would be better in the Runner (although I know space is limited in the UI now). Also, we will likely be unifying the component testing within the Desktop-GUI which would further confuse the keyboard shortcuts. Maybe we should just remove the modal in place of Integration with Travis CI #2?
  2. I'd like to see the new shortcuts documented within the tooltip of the action, similar to how the other actions show.
    Screen Shot 2021-05-13 at 11 43 35 AM
    tcut tooltips show the keyboard shortcut.
    Screen Shot 2021-05-13 at 11 43 30 AM
  3. I wouldn't worry about the pinning shortcut.

Larger discussion points

We discussed some things in our team meeting that are larger discussion points for these keyboard shortcuts in general - not specific to this PR. How this PR was implemented is fine because it follows how we originally implemented the keyboard shortcuts.

  1. The keyboard shortcuts don't work if you have a test running that is typing into the AUT OR if you manually focus an input for typing into the AUT. This pulls focus away. You can see this by running a test that does a lot of typing and trying to toggle the 'auto-focus' over and over. It just stops working.
  2. We probably should have made these keyboard shortcuts multi-key. The single keys can cause confusion if you are playing around with the AUT, interacting with inputs, then want to 'rerun' your tests for example. It's just broken.

Hi, @jennifer-shehane thanks for your review & comments!

  1. I have removed the modal for shortcut documentation. Originally I added, as you mentioned previously, because I thought the UI's space was limited. However, you're right we can update the tooltips' text instead.

  2. I have updated all relevant tooltips. You can take a look at the gifs below:

A shortcut tooltip
https://user-images.githubusercontent.com/32891808/118426667-a639f600-b699-11eb-9d43-85002b4a0aac.mov

C shortcut tooltip
https://user-images.githubusercontent.com/32891808/118426678-aa661380-b699-11eb-8ca6-85742c7f3cfc.mov

N shortcut tooltip
https://user-images.githubusercontent.com/32891808/118426694-ae923100-b699-11eb-93e1-44d28d5ffcf1.mov

  1. Alright, I wouldn't mind working on it in another PR.

As for the larger discussion points, once we merge this ticket I could work on the refactoring for this functionality to avoid the flakiness that currently exhibits, also make it into multi-keys for all key shortcuts.

We can open two new issues for this & I will be happy to address them. I think we should start with the multi-keys first & then discuss the refactoring in detail with the team. What do you think?

Let me know if there is anything else to be addressed on this PR 🙂

PS: I'm not sure why I get a couple of files with the message Empty file. in the review screen. There are no differences between the files, but somehow they make their way there. I'm not sure what's happening. Should I do something about that?

Screen Shot 2021-05-16 at 11 17 14 PM

Copy link
Member

@jennifer-shehane jennifer-shehane left a comment

Choose a reason for hiding this comment

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

I believe these 'empty file' changes show up sometimes when commits were made that changed the file, but then those changes were manually removed. So it doesn't have a diff to show, but the files were touched.

Can you add back the functionality for showing the 'next command' of the 'Next' tooltip? So, Next [N]: find. Thanks.

@Manuel-Suarez-Abascal
Copy link
Contributor Author

Manuel-Suarez-Abascal commented May 17, 2021

I believe these 'empty file' changes show up sometimes when commits were made that changed the file, but then those changes were manually removed. So it doesn't have a diff to show, but the files were touched.

Can you add back the functionality for showing the 'next command' of the 'Next' tooltip? So, Next [N]: find. Thanks.

@jennifer-shehane Oops, sorry I didn't realize we were passing the command dynamically on the tooltip's title. It's fixed now. As for the empty files, should I do something about it? Or it's all good since there're no differences?

@jennifer-shehane
Copy link
Member

Merging in, ignoring npm-design-systems failure which will maybe be fixed by #16604

@jennifer-shehane jennifer-shehane merged commit 24a4d76 into cypress-io:develop May 20, 2021
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.

Add keyboard shortcuts
2 participants