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

Remove delete prompt for empty sections #829

Conversation

leejarvis
Copy link
Contributor

If a section has no cell views or only empty cell views, avoid
prompting the user to delete the section and just go ahead and delete
it.

Added a separate hook in SessionLive to avoid invoking the delete prompt
component. I'm not sure if additional protection should be added to the
delete_empty_section callback to ensure the session is empty. Happy to
make further tweaks if there's a better way to handle this.

See #800

If a section has no cell views or only empty cell views, avoid
prompting the user to delete the section and just go ahead and delete
it.

Closes livebook-dev#800
@CLAassistant
Copy link

CLAassistant commented Dec 27, 2021

CLA assistant check
All committers have signed the CLA.

This avoids creating two separate paths in the view for displaying
delete buttons (triggering either a prompt, or deleting the empty
section).

Instead, the `delete_section` callback is always triggered, and the
"display prompt" logic is kept here.

Couple of things I'm unsure about so will discuss on the PR.

See livebook-dev#829 (comment)
@leejarvis leejarvis force-pushed the remove-prompt-for-deleting-empty-sections branch from c261cf6 to 75409ff Compare December 27, 2021 17:19
Also explicitly set the section and then re-use it. I think this is a
bit nicer than just matching against the empty list since it matches
the following match too
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.

Beautiful, thanks! 🐱

@jonatanklosko jonatanklosko merged commit 4bacba6 into livebook-dev:main Dec 27, 2021
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.

3 participants