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

87: Toggle inspector on global ALT+F12 shortcut. #92

Merged
merged 4 commits into from Jul 30, 2020
Merged

87: Toggle inspector on global ALT+F12 shortcut. #92

merged 4 commits into from Jul 30, 2020

Conversation

jodator
Copy link
Contributor

@jodator jodator commented Jul 28, 2020

So I've implemented the toggling as you proposed in the issue - that was easy.

However, I have problem with tests, see f6f1a3a. In here I've added a home made window.addEventListener() interceptor, similarly to a one proposed here.

However, the test fails on assertion (the code is executed though).


Feature: Toggle inspector on Alt+F12 keyboard shortcut. Closes #87.

@jodator jodator requested a review from oleq July 28, 2020 12:50
@jodator jodator marked this pull request as ready for review July 28, 2020 12:50
@oleq
Copy link
Member

oleq commented Jul 29, 2020

The issue with your test was that tests in this file use a mock redux data store which has a pass-through reducer so executing toggleIsCollapsed() does not change the store (store = createStore( ( state, action ) => ( { ...state, ...action.state } ), {).

All that needs to be asserted is the action dispatched on the store. What happens to the real production store on this particual action is asserted in different tests. The goal of these tests (all tests in the project) is decoupling data (store) and the view (react). 

In store tests we test the store stateA -> action dispatched on the store -> assert( store stateB ).

In component tests we test DOM event/user interaction -> assert( action type ) and fake store change -> assert( react component rendering).

I also noticed that the other test is broken (an aftermath of #94). If fixed it as well.

Copy link
Member

@oleq oleq left a comment

Choose a reason for hiding this comment

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

Have you checked if Alt+F12 works on all [Chrome, Firefox, Safari]x[Mac, Windows, popular linux WM] combinations?

@jodator
Copy link
Contributor Author

jodator commented Jul 29, 2020

Have you checked if Alt+F12 works on all [Chrome, Firefox, Safari]x[Mac, Windows, popular linux WM] combinations?

Nope. Good point. I'll check that and I'll add a better combination check - like we shouldn't act on ctrl+alt+F12.

Thanks for the explanation with tests.

@jodator
Copy link
Contributor Author

jodator commented Jul 29, 2020

So far I've checked Chrome,Firefox x Linux Mint (Ubuntu) as I don't have access to other browsers I've also searched the help.

Browsers:

And OS'es:

AFAICS most of them doesn't use "higher" F, only F11 for maximize and F12 for developer tools in FF/Chrome - so without actually testing them I'm pretty OK with this shortcut.

@jodator
Copy link
Contributor Author

jodator commented Jul 29, 2020

@oleq so I've asked @mlewand to check alt+f12 on Windows, could you do the same for Mac?

@oleq
Copy link
Member

oleq commented Jul 29, 2020

It's safe.

@jodator
Copy link
Contributor Author

jodator commented Jul 30, 2020

@oleq Tahanks!

Also, I have a confirmation from @mlewand that on Windows x (FF, Chrome, Edge) that alt+f12 it is also safe.

@oleq oleq merged commit 8059d72 into master Jul 30, 2020
@oleq oleq deleted the i/87 branch July 30, 2020 08:00
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 toggle inspector shortcut
2 participants