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

Add an options page with past runs and some controls #116

Open
wants to merge 21 commits into
base: main
Choose a base branch
from

Conversation

saranshdhingra
Copy link
Contributor

I have tried to use similar themes and components as the website.

Have run eslint on the JS files and stylelint with the same config on the CSS file.
Only non fixable errors are the incorrect selector-class-pattern in stylelint.

Apologies for the extra commits(I had taken the branch from a previously merged branch and then probably merged with the --no-ff flag)

Screens:

screen1

screen2

screen3

@saranshdhingra saranshdhingra requested a review from a team as a code owner June 17, 2022 17:30
@github-actions
Copy link

@enocom
Copy link
Member

enocom commented Jun 28, 2022

@saranshdhingra would you mind rebasing this on top of main? It will be easier to review that way ;-)

@github-actions
Copy link

Copy link
Member

@enocom enocom left a comment

Choose a reason for hiding this comment

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

Since the extension is experimental, I think this looks good.

If we were investing more time in the extension, I'd like to see us re-use the overlapping code with the webapp, and figure out how to build both the webapp and the extension using that shared code. But that's a project for another day.

Copy link
Member

@enocom enocom left a comment

Choose a reason for hiding this comment

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

Sorry just noticed we're missing some license headers in the JavaScript files.

ext/css/options.css Show resolved Hide resolved
@saranshdhingra
Copy link
Contributor Author

Since the extension is experimental, I think this looks good.

If we were investing more time in the extension, I'd like to see us re-use the overlapping code with the webapp, and figure out how to build both the webapp and the extension using that shared code. But that's a project for another day.

I would like that too. Perhaps if we spend some time on it soon, we can get that done :)

@enocom
Copy link
Member

enocom commented Jul 28, 2023

I'm no longer part of this team. Perhaps @yuryu has some thoughts on this?

ext/css/options.css Show resolved Hide resolved
@@ -0,0 +1,201 @@
window.onload = async function () {
Copy link
Member

Choose a reason for hiding this comment

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

need a license header


/**
* Fetches the details of a single historical run from chrome.storage.local
* @param {object} runId
Copy link
Member

Choose a reason for hiding this comment

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

Is this really an object? It seems to take a number.

const tabs = document
.querySelector(".tabDetails")
.querySelectorAll(".tabSingle");
[...tabs].forEach((tab, index) => {
Copy link
Member

Choose a reason for hiding this comment

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

NodeList has forEach since Chrome 51
https://developer.mozilla.org/en-US/docs/Web/API/NodeList

@@ -0,0 +1,201 @@
window.onload = async function () {
Copy link
Member

Choose a reason for hiding this comment

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

Prefer addEventListener when registering an event listener.

const runs = await getCurrentRuns();
const container = document.getElementById("runsListContainer");

runs.forEach(async (runStartTime) => {
Copy link
Member

Choose a reason for hiding this comment

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

Prefer for (... of runs)

async function getRunData(runId) {
runId = "run-" + runId;
return new Promise((resolve, reject) => {
chrome.storage.local.get(runId, (result) => {
Copy link
Member

Choose a reason for hiding this comment

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

get returns a Promise.

return (
new Intl.DateTimeFormat("en-US", { dateStyle: "long" }).format(dt) +
", " +
new Intl.DateTimeFormat("en-US", { timeStyle: "medium" }).format(dt)
Copy link
Member

Choose a reason for hiding this comment

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

Is there a reason why you specify the en-US locale instead of the user default?

* @param {object} runId
*/
async function getRunData(runId) {
runId = "run-" + runId;
Copy link
Member

Choose a reason for hiding this comment

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

I find this structure a bit confusing because the passed runId is a start time but also a run id, but you're reassigning the value with the "run-" prefix. Which format is the canonical run ID?


document
.querySelector("button.stopBtn")
.addEventListener("click", function (e) {
Copy link
Member

Choose a reason for hiding this comment

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

same, use arrow function.

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.

3 participants