-
Notifications
You must be signed in to change notification settings - Fork 6
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
feat: autostart custom notebook (#1781) #1905
Conversation
You can access the deployment of this PR at https://renku-ci-ui-1905.dev.renku.ch |
7f9f68c
to
74639b8
Compare
74639b8
to
9c954e1
Compare
9c954e1
to
0256971
Compare
0256971
to
efe32b5
Compare
efe32b5
to
69f3743
Compare
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 is very useful!
After discussions, I have some requests to make a few changes.
- The link should be available for any file in the project, not just notebooks
- For the case that the user does not want to specify the branch/commit, I think a simpler UX where the pop-up appears over the file browser would be better.
Only if the user asks to specify the branch/commit should they get redirected to the "Start notebook" page.
- Let us say which file you will return to if you do get redirected to the "Start notebook" page.
client/src/file/File.container.js
Outdated
if (this.props.filePath.endsWith(".ipynb")) | ||
buttonJupyter = (<JupyterButton {...this.props} file={filePath} />); | ||
let buttonJupyter, buttonShareLinkSession = null; | ||
if (this.props.filePath.endsWith(".ipynb")) { |
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 feature should be available for any file, not just notebooks.
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.
sure, added in the last commit
<FormGroup key="notebook-file-path"> | ||
<Label><b>Notebook:</b> {notebookFilePath}</Label> | ||
</FormGroup> | ||
) : null; |
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 think this would be nicer as text.
<FormGroup key="notebook-file-path"> | |
<Label><b>Notebook:</b> {notebookFilePath}</Label> | |
</FormGroup> | |
) : null; | |
<FormGroup key="notebook-file-path"> | |
<Label>With <b>{notebookFilePath}</b> initially open</Label> | |
</FormGroup> | |
) : null; |
@ciyer Thanks for your review, I really like the suggestion. Please find the implementation of this in the latest commit. |
8f59f3a
to
62849c0
Compare
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 works very nicely!
There are some improvements to the session start page that should be made to make this clearer, but I think it would be better to incorporate that into the broader session redesign in #1805.
Once the tests pass, I think this can be merged.
62849c0
to
7c7f30e
Compare
7c7f30e
to
a771707
Compare
a771707
to
f6d1e64
Compare
Tearing down the temporary RenkuLab deplyoment for this PR. |
PR to include the path to the notebook file in the modal to share the session link.
How it works
Closes #1781
/deploy #persist