Skip to content
This repository has been archived by the owner on Jan 24, 2021. It is now read-only.

Add keyboard shortcut to toggle night mode #62

Merged
merged 10 commits into from
Feb 16, 2018

Conversation

Ricardo-Silva91
Copy link
Contributor

@Ricardo-Silva91 Ricardo-Silva91 commented Feb 16, 2018

fixes #28

Added a new 'keyboard shortcuts' feature so that more custom shortcuts could be implemented in the future.
Also adds the custom shortcuts keys and description to the 'Keyboard Shortcuts' help modal.

To toggle night mode I just programmatically clicked the night mode element, this is a bit forceful and ugly, I'm trying to find a better solution now. (still, because the navbar is always there, this seems to work).

Feedback is welcome 😄

P.S.: the shortcut used to toggle night mode is Ctrl+m

Copy link
Owner

@sindresorhus sindresorhus left a comment

Choose a reason for hiding this comment

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

This looks great :)

case event.ctrlKey && 'm':
if (document.querySelector('.nightmode-toggle')) {
document.querySelector('.nightmode-toggle').click();
}
Copy link
Owner

Choose a reason for hiding this comment

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

Can you move this to a separate function that is called here? It's not large right now, but it will become unmaintainable like this when we add more shortcuts.

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe make keyboard-shortcuts a folder. This file would become the index and any shortcut added would have it's function on it's own file. Something like the tree below.

source
└── features
   └── keyboard-shortcuts
      ├── index.js
      └── nightmode-toggle.js

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just pushed a version with the night mode toggle in a separate function, but the 'keyboard-shortcuts folder' option does seem like a better solution.

if (document.querySelector('#init-data')) {
const initData = JSON.parse(document.querySelector('#init-data').value);
const updatedShortcuts = initData.keyboardShortcuts;
for (let i = 0; i < updatedShortcuts.length; i++) {
Copy link
Owner

Choose a reason for hiding this comment

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

for (const [i, item] of updatedShortcuts.entries()) {

}
});

if (document.querySelector('#init-data')) {
Copy link
Owner

Choose a reason for hiding this comment

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

You have document.querySelector('#init-data') 3 times. Better put it in a variable and reuse it.

Added shortcut for night mode.
fixes sindresorhus#28
@sindresorhus
Copy link
Owner

Can you add it to the feature highlights in the readme?

@sindresorhus sindresorhus changed the title keyboard shortcuts module added; night mode toggle shortcut added Add keyboard shortcut to toggle night mode Feb 16, 2018
ricardo added 2 commits February 16, 2018 18:51
Separate shortcut functions from event listener code.
Shortcut descriptions for loop.
No repeated calls to querySelector
@Ricardo-Silva91
Copy link
Contributor Author

One question: in the feature highlights in the readme, should I just say that the extension adds new keyboard shortcuts to twitter or should I list the new shortcuts as well (right now it's just the night mode toggle, but it would grow in the future)?

const initDataElement = document.querySelector('#init-data');
if (initDataElement) {
const initData = JSON.parse(initDataElement.value);
const updatedShortcuts = initData.keyboardShortcuts;
Copy link
Contributor

Choose a reason for hiding this comment

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

Make sure you spread this, so you're working with a copy instead of a reference:

const updatedShortcuts = [...initData.keyboardShortcuts];

@filipekiss
Copy link
Contributor

filipekiss commented Feb 16, 2018

@Ricardo-Silva91 for now, I guess something like "Keyboard shortcut to toggle Nightmode (Ctrl-m)" will suffice. If we ever add more shortcuts, we can change that and either list all of them in a proper section at the README.md or just tell the user to press ? on Twitter and see all shortcuts.

readme.md Outdated
@@ -31,6 +31,7 @@ We use Twitter a lot and notice many dumb annoyances we'd like to fix. So here b
- Prevents DM modal from closing when (accidentally) clicking outside the modal.
- Highlight your mentions in the stream
- [Adds a `Likes` button to the main navbar](https://user-images.githubusercontent.com/14620121/35988497-ace9f93e-0ce5-11e8-8675-17e6ee38cd99.png)
- Keyboard shortcut to toggle Night Mode (Ctrl-m).
Copy link
Contributor

Choose a reason for hiding this comment

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

Wrap keyboard shortcuts with the <kbd/> tags:

- Keyboard shortcut to toggle Night Mode (<kbd>Ctrl</kbd><kbd>m</kbd>).

readme.md Outdated
@@ -31,6 +31,7 @@ We use Twitter a lot and notice many dumb annoyances we'd like to fix. So here b
- Prevents DM modal from closing when (accidentally) clicking outside the modal.
- Highlight your mentions in the stream
- [Adds a `Likes` button to the main navbar](https://user-images.githubusercontent.com/14620121/35988497-ace9f93e-0ce5-11e8-8675-17e6ee38cd99.png)
- Keyboard shortcut to toggle Night Mode (Ctrl-m).

Tip: Twitter has a native [dark mode](https://github.com/sindresorhus/refined-twitter/issues/10). And press <kbd>Command/Ctrl</kbd> <kbd>?</kbd> to see all keyboard shortcuts.
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe update this line to show the shortcut as well?

Tip: Twitter has a native [dark mode](https://github.com/sindresorhus/refined-twitter/issues/10) and you can toggle it using <kbd>Ctrl</kbd><kbd>m</kbd>. And press <kbd>Command/Ctrl</kbd> <kbd>?</kbd> to see all keyboard shortcuts.

@filipekiss
Copy link
Contributor

Just don't forget to solve the conflicts on README.md! Besides that, you got my 👍

readme.md Outdated

Tip: Twitter has a native [dark mode](https://github.com/sindresorhus/refined-twitter/issues/10). And press <kbd>Shift</kbd> <kbd>?</kbd> to see all keyboard shortcuts.
Tip: Twitter has a native [dark mode](https://github.com/sindresorhus/refined-twitter/issues/10) and you can toggle it using <kbd>Ctrl</kbd><kbd>m</kbd>. And press <kbd>Command/Ctrl</kbd> <kbd>?</kbd> to see all keyboard shortcuts.
Copy link
Owner

Choose a reason for hiding this comment

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

And press Command/Ctrl ? to see all keyboard shortcuts.

This change is incorrect. Please see master.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right, my mistake, sorry.

}
};

export default async () => {
Copy link
Owner

Choose a reason for hiding this comment

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

Remove async. It has no purpose here.

@@ -0,0 +1,55 @@
const toggleNightMode = () => {
if (document.querySelector('.nightmode-toggle')) {
document.querySelector('.nightmode-toggle').click();
Copy link
Owner

Choose a reason for hiding this comment

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

Put it in a variable so we don't query twice.

async removed.
only 1 querySelector call on toggleNightMode.
@sindresorhus sindresorhus merged commit 4324137 into sindresorhus:master Feb 16, 2018
@sindresorhus
Copy link
Owner

Thanks for this awesome contribution @Ricardo-Silva91 :)

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Keyboard shortcut to toggle "night mode"
3 participants