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

improvement: mark commits with autodaved content when starting a new environment #1134

Merged
merged 5 commits into from
Nov 23, 2020

Conversation

lorenzo-cavazzi
Copy link
Member

Implement the solution suggested in #1113 to make it clear to the users which commits have autosaved content.

Preview: https://lorenzotest.dev.renku.ch/
How to test: Start an interactive environment on any project, modify a file and stop the environment from the environments page in the UI. Try to start a new envireonment and verify that the relevant commit is marked with a *. Select it to get a short explanation.

Screenshot_20201118_152646

Screenshot_20201118_152707

fix #1113

Copy link
Contributor

@ciyer ciyer left a comment

Choose a reason for hiding this comment

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

This is nice. Even though the '' does not have any documentation, I think users will quickly understand what it means (they will pick a commit with a '' to see why that is there, then they will see the explanation that there is unsaved work here).

I have some suggested changes to the wording and a request to make it easier to copy the text. Could we also change the title of the pop-up to: Unsaved work?

image

BTW, is it possible to indicate on the environment listing that this commit was started from an autosave branch? And could we show the command there as well?

commitComment = (
<FormText>
<FontAwesomeIcon className="no-pointer" icon={faInfoCircle} /> For this commit we
found <ExternalLink url={url} iconSup={true} iconAfter={true} title="autosaved content" role="link" />
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested wording:
We found <ExternalLink url={url} iconSup={true} iconAfter={true} title="unsaved work" role="link" /> for this commit.

@@ -1433,6 +1457,10 @@ class AutosavedDataModal extends Component {
Renku has recovered {autosavedLink} for the <i>{this.props.filters.branch.name}</i> branch.
We will automatically restore this content so you do not lose any work.
</p>
<p>
If you don&apos;t need it, you can restore the using the following command:
Copy link
Contributor

Choose a reason for hiding this comment

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

Text: If you do not need it, you can discard this work with the following command:

@@ -1433,6 +1457,10 @@ class AutosavedDataModal extends Component {
Renku has recovered {autosavedLink} for the <i>{this.props.filters.branch.name}</i> branch.
We will automatically restore this content so you do not lose any work.
</p>
<p>
If you don&apos;t need it, you can restore the using the following command:
<br /><code>git reset --hard {this.props.filters.commit.short_id} && git clean -f -d</code>
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we wrap the command in a <Clipboard>[...]</Clipboard> to make it easy to copy?

);
const docsLink = (
<ExternalLink
role="text" iconSup={true} iconAfter={true} title="documentation page"
Copy link
Contributor

Choose a reason for hiding this comment

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

Title can be just documentation (instead of "documentation page").

@lorenzo-cavazzi
Copy link
Member Author

I addressed all the comments apart from this:

BTW, is it possible to indicate on the environment listing that this commit was started from an autosave branch? And could we show the command there as well?

As discussed offline, that's trickier because we don't get that information from renku-notebooks. We determine the availability of autosaved content based on the branch names, and we get the branches directly from GitLab. Since the branch is deleted as soon as the autosaved content is restored, we don't have that information anymore. We could try to save that in the UI but a simple UI refresh would permanently remove it.
Also, the user can get rid of the autosaved content in the JupyterLab environment and the UI would still mention it. This may be confusing.

Preview updated here: https://lorenzotest.dev.renku.ch/

Screenshot_20201120_121722

Copy link
Contributor

@ciyer ciyer left a comment

Choose a reason for hiding this comment

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

Very nice!

@ciyer ciyer requested a review from rokroskar November 20, 2020 13:38
Copy link
Member

@rokroskar rokroskar left a comment

Choose a reason for hiding this comment

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

This works as advertised! The only thing I'm wondering is whether there should be some indication that some autosave branches exist at the top level of the interactive environment page - otherwise the user doesn't really know that there is some saved state until they look at the commit listing. We can discuss that separately though.

@lorenzo-cavazzi
Copy link
Member Author

That's a good point. On one hand, it may be useful information, but on the other hand, it may not be relevant to the user to know that there is autosaved content at some previous commit. If there are more recent commits, the old autosaved content is very probably outdated.

The one reason why this consideration may not apply is that we don't associate the autosaved content to the latest commit synchronized with the remote. We can expect users to keep working on environments started from outdated commits, ending up with an autosave for an old commit.

We can continue the discussion (and possibly add a mockup) here: #1143

@lorenzo-cavazzi lorenzo-cavazzi merged commit 06935b4 into master Nov 23, 2020
@lorenzo-cavazzi lorenzo-cavazzi deleted the 1113-autosave branch November 23, 2020 15:06
@rokroskar
Copy link
Member

The one reason why this consideration may not apply is that we don't associate the autosaved content to the latest commit synchronized with the remote.

yep, exactly. This is the sort of situation I got into when I was testing this PR and I wasn't sure at all how it should be resolved.

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.

Better feedback to the users for autosaved content
3 participants