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 output panel #2109

Open
wants to merge 45 commits into
base: main
Choose a base branch
from
Open

Conversation

jannikbecher
Copy link
Contributor

@jannikbecher jannikbecher commented Jul 25, 2023

(created a new PR since upstream had some changes and new implementation differs a lot from the initial one)

@github-actions
Copy link

github-actions bot commented Jul 25, 2023

Uffizzi Ephemeral Environment deployment-33510

☁️ https://app.uffizzi.com/github.com/livebook-dev/livebook/pull/2109

📄 View Application Logs etc.

What is Uffizzi? Learn more!

@jannikbecher
Copy link
Contributor Author

Some open questions:

  • order of outputs can only be changed by updating the order in the notebook, correct?
  • every output has its own row in the output panel, so the buttons to move output to the panel are obsolete?
  • outputs can only be dragged on the direct previous output?
  • basically I can use the drag&drop behaviour similar like @jonatanklosko used for file inputs without an external library?
  • where should I store the width information for the outputs?
  • does it make sense to use tailwinds column grid layout and use different breakpoints for different screen sizes?
<div class="grid grid-cols-1 md:grid-cols-6">
  <div :for={output <- outputs} class={"col-span-#{output.width}"}
    <%= output.content %>
  </div>
</div>

@jonatanklosko
Copy link
Member

jonatanklosko commented Jul 25, 2023

order of outputs can only be changed by updating the order in the notebook, correct?
every output has its own row in the output panel, so the buttons to move output to the panel are obsolete?
outputs can only be dragged on the direct previous output?

Sorry if that wasn't clear, we still need the move button, the outputs can be cherry-picked and rearranged. It's just that rather than an arbitrary grid, we want it to be a flow of rows, with an option to put several outputs in a single row.

basically I can use the drag&drop behaviour similar like @jonatanklosko used for file inputs without an external library?

I think we should be fine without a library, yeah, unless it turns out there's some specific complexity that a library could handle.

where should I store the width information for the outputs?

I would add a field to %Notebook{} like outputs_panel, and it could be a configuration like %{rows: [%{items: [%{width: 50, cell_id: "..."}]}]} (where width is a percentage, which we can quantize to be every 10 percent or similar). We will persist this differently (because cell_ids are random on every import), but we can handle this once everything else is figured out.

does it make sense to use tailwinds column grid layout and use different breakpoints for different screen sizes?

Whatever is more convenient. I think on small screens we will just make every row into a column, so each output takes the whole width.

lib/livebook/utils.ex Outdated Show resolved Hide resolved
@jannikbecher
Copy link
Contributor Author

Implemented Drag&Drop for reordering the rows:

screen-capture.10.webm

@jonatanklosko
Copy link
Member

@jannikbecher sweet! For the DND reordering we probably don't want to move the whole element, because I'm pretty sure it's not going to work well with the absolutely positioned iframes. Instead, we can drag a placeholder element, similar to this.

@jannikbecher
Copy link
Contributor Author

Something like this?

screen-capture.11.webm

@jannikbecher
Copy link
Contributor Author

Added multi column support :)
Currently the space is split up equally among all items.
When dragging on the left side of an item, it goes left. When dragging on the right side, right.
Still many UI challenges, but is it the direction you imagined?

multi_column_showcase.mp4

@josevalim
Copy link
Contributor

Would you be ok if one of us take it over the finish line in the next 2 weeks?

We pushed Livebook v0.11 to after ElixirConf US, so we have a month until next release, so no rush from our side :)

@jannikbecher jannikbecher marked this pull request as ready for review August 15, 2023 15:36
@josevalim
Copy link
Contributor

Hi @jannikbecher! Just a heads up our release probably be on the week of Sep 18th. However we will likely be tidying up features in the week of Sep 11th. :)

@jannikbecher
Copy link
Contributor Author

hey @josevalim. Thanks for the update.
Could you provide feedback on what needs to be improved for this to be merged into the main branch? :)

@josevalim
Copy link
Contributor

josevalim commented Aug 31, 2023

@jannikbecher this looks amazing. I have bunch of nitpicks for us to iron out the final details (fwiw I have tested these on Firefox).

  1. Instead of showing the the trash icon along-side the drag and drop icon, show the logout box icon, since we don't really delete it, simply we put it back

  2. When I mouse over an element in the panel, scrollbars appear (see image below)

    Screenshot 2023-08-31 at 20 57 15
  3. When I start to drag and drop, a blue bar is used to represent the element, but I also have a blue bar in the same place as the drag and drop button (see image below)

    Screenshot 2023-08-31 at 20 58 50
  4. Can we unify the row and column drag and drops? Today I need to move something to a row and then move the columns. It would be nice if we could move everything around with a single drag and drop, something like this:

    |---------------------|
    |                     |
    |        row 1        |
    |                     |
    |---------------------|
      
    |----------|----------|
    |  row 2A  |  row 2B  |
    |----------|----------|
    

    Now imagine I move drag and drop row 2A. If I move a little bit up, we will see an area called new row which we could drop into:

    |---------------------|
    |                     |
    |        row 1        |
    |                     |
    |---------------------|
    |------ new row ------|
    |----------|----------|
    |  row 2A  |  row 2B  |
    |----------|----------|
    

    We need some "natural space" between rows but, once we mouse over it during drag and drop, it gets some height (and some color).

    If I move it a bit further up, we will see this:

    |---------------------|
    | |-------| |-------| |
    | |      r|w|1      | |
    | |-------| |-------| |
    |---------------------|
    
    |----------|----------|
    |  row 2A  |  row 2B  |
    |----------|----------|
    

    In other words, it will overlay two squares on top of the current content on row 1. If the drag and drop is on the left, we will show "New column" on the left, if on the right, we will show "New column" there.

I have navigated https://datasciencenotebook.org/ looking for inspirations and I think Hex' App Builder is the closest to what you are building, in case you are looking for inspiration in this final stretch. :)

@josevalim
Copy link
Contributor

Continuing:

  1. Keep in mind that there is a button called "Clear outputs" (on the triple dots close to the notebook title), which means the outputs can be cleared at any time (this will also happen if you import a notebook). In such cases, we should probably show a box saying "This cell was not yet rendered". if you want to be fancy, you could even have a evaluate button alongside the drag and drop/delete buttons (we could leave this button permanently there. If the cell was never evaluted, we show the "play" icon. if it was evaluated, we can show the "memories" remix icon)

  2. Because of the above, the export button (login/logout) should always be visible, even if the cell was not evaluated

  3. There is additional extra space in the deploy modal if "Output panel" is chosen:

    Screenshot 2023-08-31 at 21 46 29
  4. Btw, I think we should change "Output type" to radio. It means users can see all options without an additional click. Especially given we only have three options


@jonatanklosko, quick question for you: what do we do with the placement of the export button (login/logout icons)? Currently it is on the right, but we have avoided adding icons to the right so the delete icon is always aligned. At the same time, I think on the right is the best place. What to do? Maybe we should allow Markdown to be sent to the output pane too? 🤐 It would make things consistent but maybe way more complex.

@jonatanklosko
Copy link
Member

jonatanklosko commented Aug 31, 2023

Fantastic! 🚀

  1. When I add a second output, the first one gets duplicated. The same happens after drag and drop. Refreshing gets it back to normal. // Ah, this is because we don't prune outputs in the new LV, you don't have to worry about it, I can fix this later :)
panel_duplicate.mov
  1. When popping-out the iframe is misplaced.
panel_iframe.mov
  1. Once the panel is open all markdown cells are gone from the notebook.

@josevalim I wouldn't allow markdown. It is additional complexity and since it's "output panel" allowing only outputs makes more sense (we could say rendered markdown is markdown cell's output, but I don't like blurring that boundary). I see why the rightmost placement makes sense, but at the same time putting it next to the trash icon isn't necessarily the best, because if people are using it often it's more likely they will accidentally remove cells. I'm fine placing it before the link icon to keep the alignment we've maintained so far.

@josevalim
Copy link
Contributor

@jonatanklosko alright. let's leave the button where it is for now and we can bikeshed after merging. :)

@josevalim
Copy link
Contributor

Hi @jannikbecher, just a quick check to see where we are at. :) If you want to us to wrap it up, feel free to let us know too :)

@jannikbecher
Copy link
Contributor Author

hey @josevalim :) sorry for the late response.
Today I will be able to continue the work and hopefully fix the remaining issues 👍

@josevalim
Copy link
Contributor

No worries, we just did the priorization for Livebook v0.11 and this feature can wait until v0.12 if you need more time. :)

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.

4 participants