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

Add notebook option for persisting outputs #485

Merged
merged 2 commits into from
Jul 30, 2021
Merged

Conversation

jonatanklosko
Copy link
Member

Closes #206.

Adds a new notebook :persist_outputs attribute (persisted as metadata). The attribute can be set in the persistence modal. Once set, outputs are persisted the same way as in export (#483).

livemd_with_output.mp4

@jonatanklosko
Copy link
Member Author

jonatanklosko commented Jul 30, 2021

@josevalim I think we need to keep the notebook metadata comment at the top rather than bottom. One point is consistency, since all other meta comments are said to refer to the elements they precede. Also, a meta comment at the end may refer to an empty Markdown cell (currently we don't have any metadata for such, but we may have), or it could be livebook:{"break_markdown":true}, so the subject of a trailing meta would be ambiguous. Given it's just a single line, I don't think it's particularly problematic to keep it in the first line, especially that notebooks don't have any metadata by default in the first place.

attrs = %{unknown: :value}
operation = {:set_notebook_attributes, self(), attrs}

assert :error = Data.apply_operation(data, operation)
Copy link
Contributor

Choose a reason for hiding this comment

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

What does the UI do in this case? Just fails to import it? Perhaps we should print an error message?

Copy link
Member Author

Choose a reason for hiding this comment

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

This isn't used for importing, it's just an operation similar to :set_cell_attributes that updates the session data we have. These attributes are usually updated via UI in one way or another and we control what these are. Technically we could raise an error, though returning :error this is more in line with all other errors.

Copy link
Contributor

@josevalim josevalim left a comment

Choose a reason for hiding this comment

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

Beautiful!

Quick question: are we already importing them?

also, what do you think about moving the checkbox closer to the bottom just above the save button on the save screen? I think you can also increase the vertical spacing there, the buttons and controls are very close to each other. :)

@jonatanklosko
Copy link
Member Author

Quick question: are we already importing them?

Correct, we import each output snippet as {:text, snippet_content}.

what do you think about moving the checkbox closer to the bottom just above the save button on the save screen?

It feels more natural to place global options above, since clicking the buttons shows conditional content and the checkbox would jump unnecessarily.

@josevalim
Copy link
Contributor

Sounds good. Last question: what happens when there is an output cell not after an Elixir cell? Do we discard it? Treat it as markdown? If this is handled, ship it!

@jonatanklosko
Copy link
Member Author

what happens when there is an output cell not after an Elixir cell?

When importing, once we encounter Elixir cell, we then take all subsequent output snippets as output, so snippets anywhere else are treated as Markdown (they use output language so it's rather unlikely anyway).

@jonatanklosko
Copy link
Member Author

I think you can also increase the vertical spacing there, the buttons and controls are very close to each other. :)

WDYT?

image

@josevalim
Copy link
Contributor

We need some spacing between the file text and the button, as it looks inconsistent with the rest, otherwise looks good!

@josevalim
Copy link
Contributor

You can also look at the size we are using in other similar screens, such as the runtime one, so we make it all consistent. :)

@jonatanklosko
Copy link
Member Author

We need some spacing between the file text and the button, as it looks inconsistent with the rest, otherwise looks good!

Done!

You can also look at the size we are using in other similar screens, such as the runtime one, so we make it all consistent. :)

That's what I usually do, although a single spacing doesn't fit all types of content. For example in the runtime modal we have quite a lot of text or similar controls, in which case large spacing looks weird :)

@jonatanklosko jonatanklosko merged commit 634907b into main Jul 30, 2021
@jonatanklosko jonatanklosko deleted the jk-save-outputs branch July 30, 2021 14:24
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.

Allow .livemd to be saved with result and output
2 participants