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

Bulk actions for sessions #939

Merged

Conversation

cristineguadelupe
Copy link
Contributor

@cristineguadelupe cristineguadelupe commented Jan 26, 2022

Closes #914

Screen.Recording.2022-01-28.at.17.20.17.mov

@cristineguadelupe cristineguadelupe changed the title Cg sessions bulk actions Bulk actions for sessions Jan 26, 2022
@josevalim
Copy link
Contributor

josevalim commented Jan 26, 2022

This is looking great! Some feedback:

  1. Let's rename "Disconnect session" to "Disconnect runtime". For the icon, let's use the same icon for the runtime in the notebook sidebar.

  2. For consistency, let's also add "Disconnect runtime" to the individual icon here:

    Screenshot 2022-01-26 at 12 12 45

  3. When I said we should have two buttons, I was thinking we would only have two actions: Cancel and Close. But right now we have multiple actions so I would stick with a single button called [ Actions ]. Inside the button we will have the following actions "Cancel", "Select all", "Disconnect runtime", and "Close sessions". If nothing is selected, both "Disconnect runtime" and "Close sessions" should show up disabled

  4. Minor nitpick: the alignment of "Runtime sessions" with the button looks weird. Maybe we should move the button to the right side? Either before or after the "Sorting" button.

EDIT:

  1. In "Close session" confirmation window, if any of the sessions we are closing were not saved to disk, we should explicitly say so. I don't think we need to list the name of the sessions. So I would go with something like this:

    Are you sure you want to close 5 sections?
    Important: 3 notebooks are not persisted and their content will be lost.

@cristineguadelupe cristineguadelupe marked this pull request as ready for review January 26, 2022 18:55
Copy link
Member

@jonatanklosko jonatanklosko left a comment

Choose a reason for hiding this comment

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

Fantastic work using LiveView.JS! A couple more comments from me 🐱

lib/livebook_web/live/home_live.ex Outdated Show resolved Hide resolved
lib/livebook_web/live/home_live/session_list_component.ex Outdated Show resolved Hide resolved
lib/livebook_web/live/home_live/session_list_component.ex Outdated Show resolved Hide resolved
test/livebook_web/live/home_live_test.exs Outdated Show resolved Hide resolved
test/livebook_web/live/home_live_test.exs Outdated Show resolved Hide resolved
assets/js/app.js Outdated Show resolved Hide resolved
assets/js/app.js Outdated Show resolved Hide resolved
lib/livebook_web/live/home_live/session_list_component.ex Outdated Show resolved Hide resolved
assets/js/app.js Outdated Show resolved Hide resolved
assets/js/app.js Outdated Show resolved Hide resolved
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.

Looks great to me! I have only dropped very minor comments.

However, I think we need a test that goes through the process of:

  1. Starting a runtime
  2. Closing it off via a bulk action
  3. Verifying the runtime is gone

Once that is ready, this is all done!

Copy link
Member

@jonatanklosko jonatanklosko left a comment

Choose a reason for hiding this comment

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

Awesome job! 🐱

@josevalim?

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.

Absolutely beautiful! I have dropped one very tiny comment!

@cristineguadelupe cristineguadelupe merged commit 4dd2838 into livebook-dev:main Jan 28, 2022
@cristineguadelupe cristineguadelupe deleted the cg-sessions-bulk-actions branch January 28, 2022 20:45
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 multiple sessions to be closed at once in the home page
3 participants