-
Notifications
You must be signed in to change notification settings - Fork 2
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 commands to open URL in new window, and to deploy App in new window #7
Conversation
style/base.css
Outdated
@@ -17,3 +17,10 @@ | |||
outline-offset: -3px; | |||
outline-style: auto; | |||
} | |||
|
|||
.nebari-DeployAppIcon { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The CSS needs to be fixed - currently it only displays when I give fixed size (e.g. 16px
).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great start! Do you plan to add the button to the toolbar and context menu in this PR or subsequent one?
src/index.ts
Outdated
}); | ||
}, | ||
label: () => 'Deploy App', | ||
iconClass: () => 'nebari-DeployAppIcon' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ideally we would use proper icon
(an instance of LabIcon
) here - iconClass
is de-facto deprecated. The less CSS rules the better for performance.
See
https://github.com/nebari-dev/jupyterlab-gallery/blob/ef56fc616bae4ec8305c01e3341b773ca4cfd48f/src/icons.ts#L5-L8 and https://github.com/nebari-dev/jupyterlab-gallery/blob/ef56fc616bae4ec8305c01e3341b773ca4cfd48f/src/svg.d.ts
src/index.ts
Outdated
app.commands.addCommand(CommandIDs.openURL, { | ||
execute: async (args: IOpenURLArgs) => { | ||
const { url } = resolveURLAndAlias(args); | ||
try { | ||
window.open(url, '_blank', 'noopener,noreferrer'); | ||
} catch (error) { | ||
console.warn('Error opening URL:', url, error); | ||
return; | ||
} | ||
}, | ||
label: (args: IOpenURLArgs = {}) => { | ||
const { alias } = resolveURLAndAlias(args); | ||
return `Open ${alias}`; | ||
} | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would suggest removing the command for opening an arbitrary URL to reduce the API surface
@krassowski Quick follow-up:
I was under the impression that these would be configured in the settings in What's the usual practice? That an extension includes these settings by default, and then a user (e.g. |
I think its better to include them here in a single plugin rather than in |
Yes, it is much easier to:
than if we were to split the logic between the two. |
Just wanted to mention that we need to keep in mind that jhub-apps is an optional feature of Nebari and therefore the changes we introduce here should also be safe for when jhub-apps is not present. (Maybe this is already true, I dont know about the technical details here 😀) |
schema/jhub-apps-integration.json
Outdated
"context": [ | ||
{ | ||
"command": "jhub-apps:deploy-app", | ||
"selector": ".jp-DirListing-item", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Question: Does selector
really need to be here (it seems so), and if so, what should I set it as?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. I think that for now it only makes sense for files not for directories so the selector should reflect that.
src/index.ts
Outdated
}, | ||
label: () => 'Deploy App', | ||
icon: () => { | ||
// Add logic to only display the icon if the command is not in the main menu |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a way to do this i.e. display the icon only except for the main menu (see the screenshots and compare them to the Figma diagrams)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This method takes an argument that can be used to display this icon or not - I don't remember all attributes of top of my head but it should have something like isToolbar
and the underlying event.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, I've just added a new argument (origin
) to the schema
JSON that controls this, I couldn't figure out how to get something like isToolbar
working.
Closing in favour of nebari-dev/jupyterlab-jhub-apps#1 that was just merged :) 🎉 |
This PR adds a new command:
deploy-app
.deploy-app
deploy-app
page in a new browser tab. Also passes a query parameter forfilepath
that is generated from the current workspace in JLab.Reference Issues or PRs
nebari-dev/jhub-apps#472
What does this implement/fix?
Put a
x
in the boxes that applyTesting
Documentation
Access-centered content checklist
Text styling
H1
or#
in markdown).Non-text content
Any other comments?