Add an options page with past runs and some controls#116
Add an options page with past runs and some controls#116saranshdhingra wants to merge 21 commits intomainfrom
Conversation
|
@saranshdhingra would you mind rebasing this on top of main? It will be easier to review that way ;-) |
enocom
left a comment
enocom
left a comment
There was a problem hiding this comment.
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.
enocom
left a comment
enocom
left a comment
There was a problem hiding this comment.
Sorry just noticed we're missing some license headers in the JavaScript files.
I would like that too. Perhaps if we spend some time on it soon, we can get that done :) |
|
I'm no longer part of this team. Perhaps @yuryu has some thoughts on this? |
| @@ -0,0 +1,201 @@ | |||
| window.onload = async function () { | |||
|
|
||
| /** | ||
| * Fetches the details of a single historical run from chrome.storage.local | ||
| * @param {object} runId |
There was a problem hiding this comment.
Is this really an object? It seems to take a number.
| const tabs = document | ||
| .querySelector(".tabDetails") | ||
| .querySelectorAll(".tabSingle"); | ||
| [...tabs].forEach((tab, index) => { |
There was a problem hiding this comment.
NodeList has forEach since Chrome 51
https://developer.mozilla.org/en-US/docs/Web/API/NodeList
| @@ -0,0 +1,201 @@ | |||
| window.onload = async function () { | |||
There was a problem hiding this comment.
Prefer addEventListener when registering an event listener.
| const runs = await getCurrentRuns(); | ||
| const container = document.getElementById("runsListContainer"); | ||
|
|
||
| runs.forEach(async (runStartTime) => { |
| async function getRunData(runId) { | ||
| runId = "run-" + runId; | ||
| return new Promise((resolve, reject) => { | ||
| chrome.storage.local.get(runId, (result) => { |
| return ( | ||
| new Intl.DateTimeFormat("en-US", { dateStyle: "long" }).format(dt) + | ||
| ", " + | ||
| new Intl.DateTimeFormat("en-US", { timeStyle: "medium" }).format(dt) |
There was a problem hiding this comment.
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; |
There was a problem hiding this comment.
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) { |
3 participants
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-patternin stylelint.Apologies for the extra commits(I had taken the branch from a previously merged branch and then probably merged with the
--no-ffflag)Screens: