Skip to content
This repository has been archived by the owner on Sep 26, 2024. It is now read-only.

MOC-226: play mode icon #164

Merged
merged 8 commits into from
Aug 28, 2024
Merged

MOC-226: play mode icon #164

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
514e744
add play icon
Aug 28, 2024
a8a77e3
cleanup
Aug 28, 2024
db7324e
mo cleanup
Aug 28, 2024
33c1b85
a little more cleanup
Aug 28, 2024
59e22cf
logging
Aug 28, 2024
84f3ca6
editing
Aug 28, 2024
fc4bbca
fix regression from cleanup
Aug 28, 2024
4c4fc22
PR feedback
Aug 28, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Binary file added apps/mocksi-lite-next/public/play-icon.png
View file
Open in desktop
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
40 changes: 39 additions & 1 deletion apps/mocksi-lite-next/src/pages/background/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,12 @@
import { jwtDecode } from "jwt-decode";

console.log("background script loaded");

const MOCKSI_AUTH = "mocksi-auth";
let prevRequest = {
data: {},
message: "INIT",
};

const getAuth = async (): Promise<null | {
accessToken: string;
Expand Down Expand Up @@ -80,6 +85,18 @@ addEventListener("install", () => {
chrome.action.onClicked.addListener((tab) => {
if (tab.id) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Lets get rid of the tab.id nesting by refactoring this to:

if (!tab?.id) {
  console.log("No tab found, could not mount extension");
  return;
}

chrome.tabs.sendMessage(tab.id, { message: "mount-extension" });

if (prevRequest.message) {
  chrome.tabs.sendMessage(tab.id, {
    data: prevRequest.data,
    message: prevRequest.message,
  });
}

if (prevRequest.message === "PLAY") {
  chrome.action.setIcon({
    path: "play-icon.png",
    tabId: tab.id,
  });
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

nice

chrome.tabs.sendMessage(tab.id, { message: "mount-extension" });
if (prevRequest.message) {
chrome.tabs.sendMessage(tab.id, {
data: prevRequest.data,
message: prevRequest.message,
});
}
if (prevRequest.message === "PLAY") {
chrome.action.setIcon({
path: "play-icon.png",
tabId: tab.id,
});
}
} else {
console.log("No tab found, could not mount extension");
}
Expand All @@ -98,7 +115,10 @@ chrome.runtime.onMessage.addListener(

chrome.runtime.onMessageExternal.addListener(
(request, _sender, sendResponse) => {
console.log("Received message from external:", request);
// This logging is useful and only shows up in the service worker
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

console.log(" ");
console.log("Previous message from external:", prevRequest);
console.log("Received new message from external:", request);

// execute in async block so that we return true
// synchronously, telling chrome to wait for the response
Expand Down Expand Up @@ -132,6 +152,19 @@ chrome.runtime.onMessageExternal.addListener(
console.log("No active tab found, could not send message");
return true;
}

if (request.message === "PLAY") {
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's refactor this code to use a switch statement for better readability and scalability:

switch (request.message) {
  case "PLAY":
    await chrome.action.setIcon({
      path: "play-icon.png",
      tabId: tab.id,
    });
    break;
    
  case "MINIMIZED":
    // No action needed for "MINIMIZED"
    break;

  default:
    chrome.action.setIcon({
      path: "mocksi-icon.png",
      tabId: tab.id,
    });
    break;
}

This refactor handles the different cases of request.message more cleanly. If there are more cases to handle in the future, adding them to the switch statement will be much easier.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

for sure

await chrome.action.setIcon({
path: "play-icon.png",
tabId: tab.id,
});
} else if (request.message !== "MINIMIZED") {
chrome.action.setIcon({
path: "mocksi-icon.png",
tabId: tab.id,
});
}

chrome.tabs.sendMessage(
tab.id,
{
Expand All @@ -145,6 +178,11 @@ chrome.runtime.onMessageExternal.addListener(
}
})();

// Store last app state so we can return to the correct state when the
// menu is reopened
if (request.message !== "MINIMIZED") {
prevRequest = request;
}
return true;
},
);
34 changes: 22 additions & 12 deletions apps/mocksi-lite-next/src/pages/content/mocksi-extension.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import { AppliedModifications, Reactor } from "@repo/reactor";
import type { ModificationRequest } from "@repo/reactor";
import { Reactor } from "@repo/reactor";
import React from "react";
import ReactDOM from "react-dom";
import { createRoot } from "react-dom/client";
Expand Down Expand Up @@ -91,6 +91,7 @@ chrome.runtime.onMessage.addListener((request) => {
) {
reactor.detach();
}

// resize iframe
if (iframeRef.current) {
let styles = {};
Copy link
Contributor

Choose a reason for hiding this comment

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

Hey, awesome job on this! I wanted to suggest a small tweak that could make the code a bit cleaner. I’ve found that when an if statement goes over five lines, it’s usually a sign that it could benefit from some refactoring. It just keeps things more readable and easier to maintain.

For the part where you’re setting styles based on request.message, we could simplify things by moving that logic into a separate function. This way, the main code stays focused, and it’ll be easier to update or debug if we need to adjust the styles down the line.

Here’s a quick refactor idea:

// Function to get styles based on the message
function getIframeStyles(message: string): Partial<CSSStyleDeclaration> {
  switch (message) {
    case "ANALYZING":
    case "PLAY":
    case "RECORDING":
      return {
        display: "block",
        height: "150px",
        inset: "0px 10px auto auto",
        width: "400px",
      };

    case "MINIMIZED":
      return {
        display: "none",
        inset: "0px 0px auto auto",
      };

    case "EDITING":
    case "INIT":
    case "LIST":
    case "READYTORECORD":
    case "SETTINGS":
    case "STOP_EDITING":
    case "STOP_PLAYING":
    case "UNAUTHORIZED":
      return {
        display: "block",
        height: "600px",
        inset: "auto 10px 10px auto",
        width: "500px",
      };

    default:
      return {};
  }
}

// Resize iframe with the new styles
if (iframeRef.current) {
  const styles = getIframeStyles(request.message);
  Object.assign(iframeRef.current.style, styles);
}

This should help keep things more modular and easier to manage. Let me know what you think!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

for sure changed, I have plans to clean this up

Expand All @@ -99,25 +100,33 @@ chrome.runtime.onMessage.addListener((request) => {
case "PLAY":
case "RECORDING":
styles = {
bottom: "auto",
display: "block",
height: "150px",
top: "0px",
/* top right bottom left */
inset: "0px 10px auto auto",
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: Remove the comment /* top right bottom left */

Tip: To remember the order of these CSS properties, think of it as moving clockwise like on a clock face: starting at 12 (top), then 3 (right), 6 (bottom), and finally 9 (left).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

for sure removed

width: "400px",
};
break;
case "MINIMIZED":
styles = {
bottom: "auto",
height: "0",
top: "auto",
width: "0",
display: "none",
/* top right bottom left */
inset: "0px 0px auto auto",
};
break;
default:
case "EDITING":
case "INIT":
case "LIST":
case "READYTORECORD":
case "SETTINGS":
case "STOP_EDITING":
case "STOP_PLAYING":
case "UNAUTHORIZED":
styles = {
bottom: "10px",
display: "block",
height: "600px",
top: "auto",
/* top right bottom left */
inset: "auto 10px 10px auto",
width: "500px",
};
}
Expand All @@ -142,16 +151,17 @@ chrome.runtime.onMessage.addListener((request) => {
{ReactDOM.createPortal(
<>
<iframe
loading="lazy"
ref={iframeRef}
seamless={true}
src={`${import.meta.env.VITE_NEST_APP}/extension`}
style={{
colorScheme: "light",
position: "fixed",
bottom: "10px",
right: "15px",
display: "block",
height: "600px",
width: "500px",
inset: "auto 10px 10px auto",
boxShadow: "none",
zIndex: 99998,
border: "none",
Expand Down
Loading