Popout changes #7
Conversation
There was a problem hiding this comment.
Thank you so much for working on this, @ranashreyas! I've left a bunch of code comments. I've not been able to test this yet - do you have a screenshot that may help me see the changes?
| @@ -7,3 +7,13 @@ chrome.tabs.onUpdated.addListener((tabId, changeInfo, tab) => { | |||
| chrome.action.setPopup({ tabId, popup: "popup_disabled.html" }); | |||
| }; | |||
| }); | |||
|
|
|||
| chrome.runtime.onInstalled.addListener(function (object) { | |||
There was a problem hiding this comment.
Can this be a page on this repository instead? Chrome and firefox both review URLs used in here, and I think they may not be happy with this one :)
There was a problem hiding this comment.
Google Docs is definitely better than notion! However, can you copy the contents into a markdown file in this repository (as part of this PR), and link to that instead?
src/index.jsx
Outdated
| if (activeTab) { | ||
| const parts = GitUrlParse(activeTab.url); | ||
| const repoUrl = `${parts.protocol}://${parts.source}/${parts.full_name}`; | ||
| const my_url = generateRegularUrl(hubUrl, repoUrl, parts.ref, app, parts.name + '/' + parts.filepath); |
There was a problem hiding this comment.
variables should be camelCase, so myUrl not my_url
There was a problem hiding this comment.
I also see that this is mostly duplicating the code in copyGeneratedUrl. I see there are two issues here:
- this is also copying the URL, in addition to opening it
- The code duplication may become a problem. Ideally, the common part of these two functions would be refactored into another one that can be called here.
src/index.jsx
Outdated
|
|
||
| const changeOpenState = (item) => { | ||
| if(isDropdownOpen == true){ | ||
| document.getElementById("root").style.height="190px"; |
There was a problem hiding this comment.
I'd prefer we use something like CSS classes here, rather than directly setting CSS properties in JS.
There was a problem hiding this comment.
If this becomes small after moving these to a css class toggle, this function can also be inline.
src/index.jsx
Outdated
| <Heading sx={{ fontSize: 2, mb: 1, mt: 2 }}>Open in</Heading> | ||
| <select className="form-select mb-1" onChange={(ev) => setApp(ev.target.value)} value={app}> | ||
| <Heading sx={{ fontSize: 2, mb: 1, mt: 3 }}>Open in</Heading> | ||
| {/* <select className="form-select mb-1" onChange={(ev) => setApp(ev.target.value)} value={app}> |
src/index.jsx
Outdated
| key: name | ||
| })); | ||
|
|
||
| const handleMenuChange = (item) => { |
There was a problem hiding this comment.
Given this is so small, I think this can just be inline.
src/popup.html
Outdated
| @@ -1,9 +1,10 @@ | |||
| <html> | |||
| <html style="height: fit-content;"> | |||
src/popup.html
Outdated
| <div id="root" style="width:320px; height:200px;"></div> | ||
| <body id="popoutBody"> | ||
| <div id="root" style="width:370px; height:190px;"></div> | ||
| <!-- <div id="root" style="width:320px; height:200px;"></div> --> |
|
hey @yuvipanda @balajialg I've made most of these changes, and I've pushed the latest version. Please let me know if there should be more changes or if it's ready to merge and publish. |
| @@ -7,3 +7,13 @@ chrome.tabs.onUpdated.addListener((tabId, changeInfo, tab) => { | |||
| chrome.action.setPopup({ tabId, popup: "popup_disabled.html" }); | |||
| }; | |||
| }); | |||
|
|
|||
| chrome.runtime.onInstalled.addListener(function (object) { | |||
There was a problem hiding this comment.
Google Docs is definitely better than notion! However, can you copy the contents into a markdown file in this repository (as part of this PR), and link to that instead?
|
@yuvipanda @balajialg I've made some further changes, such as removing inline CSS and making the instructions tab a Markdown file in the repository. This also uses a styled html element instead of using Primer's version. |
|
@ranashreyas Thanks! Can you please resolve @yuvipanda's comments that you already addressed as part of the latest PR |
Icon and Rebrands
@yuvipanda @balajialg I've included some changes to the popout banner, such spacing, coloring, etc. It still follows similar stylistic choices and uses Primer. There is also a new button labeled "open in new tab" which directly opens the files in the Jupyter in a new tab. On first install, it opens a Notion page which gives steps and instructions on how to pin the extension to the toolbar, along with instructions on how to use it.
All new functionality works fine in both Chrome and Firefox.