Fix #1503: Fix skip track buttons on mac devices

ajkapala21 commented 2 months ago

In issue #1503 the the issuer noted that the usual keyboard buttons to switch to the next track or previous (or play/pause) do not work. The buttons they are speaking of are shown here https://www.macworld.com/wp-content/uploads/2023/01/mac-keyboard-play-pause-100837087-orig.jpg?quality=50&strip=all&w=1024. From searching online it appears that all apple keyboards include these symbols to suggest that the f7, f8, and f9 keys have functionalities to switch to the previous track, play/pause, and switch the next track respectively.

This change adds these shortcuts for only mac devices, since this functionality is only suggested prominently on mac devices.

nuki-chan[bot] commented 2 months ago

Lines 1-32 in `ShortcutsContainer.test.tsx`

+  it('should start playing the current track when the f8 key is clicked on mac', async () => {
+    // ... test code ...
+  });
+
+  it('should pause the current track when the f8 key is clicked and the song is currently playing on mac', async () => {
+    // ... test code ...
+  });
+
+  it('should skip to the next track when the f9 is clicked on mac', async () => {
+    // ... test code ...
+  });
+
+  it('should skip to the previous track when the f7 is clicked on mac', async () => {
+    // ... test code ...
+  });
+  // ... rest of the code ...

Sugoi! I spy with my little eye, some brand-new test cases! Or at least I hope they are, 'cause we all love some well-tested code, ne? 💻✨ But Nuki-chan has a couple of itsy-bitsy critiques to make your tests even more kawaii! 🌸🎀 I know, patience young coder!

Ohayou! You're relying on isMac() to dictate the test flow. I see you're an "if-this-then-that" aficionado! But wouldn't it be totally kawaii to have deterministic tests? Mock the isMac function to return true for one set of tests and false for another, so no matter what planet the tests are running on, they'll pass! 😉
Additionally, Konichiwa! Each test should be focused on one thing like Nuki-chan focuses on sweets! 🍩 Consider splitting them into two, one for Mac and one for, you know, the rest of the peasants – I mean users. ✨

Here's how you might split one test and mock isMac:

// Line 12 and 28 can be replaced with this code block.

// This is for the Mac test, ne~?
jest.mock('../../hooks/usePlatform', () => ({
  isMac: () => true,
}));

it('should start playing the current track when the f8 key is clicked on Mac', async () => {
  // ... test code for Mac exclusive ...
});

// And this one is for Non-Mac users, mwahaha!
jest.mock('../../hooks/usePlatform', () => ({
  isMac: () => false,
}));

it('should not start playing the current track when the f8 key is clicked on non-Mac', async () => {
  // ... test code for non-Mac exclusives ...
});

Remember, Nuki says keep your tests as predictable as that drama series finale that you binge-watched last weekend! No surprises! 📺✨

Lines 137-140 in `ShortcutsContainer/index.js`

+    if (isMac()){
+      Mousetrap.bind('f7', this.props.actions.previousSong);
+      Mousetrap.bind('f8', this.handleSpaceBar);
+      Mousetrap.bind('f9', this.props.actions.nextSong);
+    }

Nuki-chan smiles on this one! 😺 Good job for checking for Mac specific keybindings. But does it hurt to be a tad bit more consistent? I mean, that if statement looks as out of place as a cat at a dog parade. 🐾 Consider encapsulating the Mac-specific binds in their own kireina function, and call it within componentDidMount(). Coding is art, and consistency is the golden frame, right? ✨

// Line 137-140 can be replaced with this snippet.

registerMacShortcuts() {
  Mousetrap.bind('f7', this.props.actions.previousSong);
  Mousetrap.bind('f8', this.handleSpaceBar);
  Mousetrap.bind('f9', this.props.actions.nextSong);
}

componentDidMount() {
  // ... other bindings ...
  if (isMac()) {
    this.registerMacShortcuts();
  }
}

Also, Nuki-chan wants you to pay attention when you unmount those keybindings. Don't leave them hanging like a clingy ex! Unbind them properly in componentWillUnmount() 💔

Lastly, let's not forget what you have bound, lest you end up with a jigsaw puzzle of forgotten functions! Make a note, or even better, have an array/object to keep track, ne? Organization is kawaii! 📌✨

GANBATTE! Let's make that code as purr-fect as the code-reviewing anime girl Nuki is! 😸💕

nukeop commented 2 months ago

Yeah, I agree with Nuki - isMac function should be mocked and both cases should be covered in tests.

ajkapala21 commented 2 months ago

Just committed the changes for the tests to follow your recommendation.

nukeop / nuclear