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

Allow dropping external files into the notebook #2097

Merged
merged 4 commits into from
Jul 22, 2023
Merged

Allow dropping external files into the notebook #2097

merged 4 commits into from
Jul 22, 2023

Conversation

jonatanklosko
Copy link
Member

dnd_external.mp4

@github-actions
Copy link

github-actions bot commented Jul 21, 2023

Uffizzi Preview deployment-31543 was deleted.

@josevalim
Copy link
Contributor

Some nitpicks:

  1. We have "Add file" and then "Insert file", those are very similar. I think we need a different name for the second one. "Embed into notebook" or "Process file" or similar (I like the latter)

  2. The upload validation requires focus to be removed to emit errors (or enable the "Add" button):

Screenshot 2023-07-21 at 22 02 04
  1. Can you also add some margin-bottom/padding-bottom to "What do you want to do with the file?" (mb-4 works) (sorry)

@josevalim
Copy link
Contributor

Also note that the flow is a bit broken if the runtime is not started. We add the file, then we ask to start the runtime, but then we never go back to the process file. Should we ask to start the runtime before we add it?

Also, should we allow the file to be added directly to the "files sidebar"?

Also, what about elixir and markdown files? Those don't need to be added to the files sidebar, as they can just become regular markdown or code cells. Thoughts?

@jonatanklosko
Copy link
Member Author

We have "Add file" and then "Insert file", those are very similar. I think we need a different name for the second one. "Embed into notebook" or "Process file" or similar (I like the latter)

I see, though "Insert" generally is about putting something in the notebook content, we insert images, cells. If you insert image as markdown cell, it's not really processing, so I'm not really sure.

The upload validation requires focus to be removed to emit errors (or enable the "Add" button):

The errors are there, but LV keeps the errors hidden until the file is changed on the client. I added event dispatch to simulate input.

Also note that the flow is a bit broken if the runtime is not started. We add the file, then we ask to start the runtime, but then we never go back to the process file. Should we ask to start the runtime before we add it?

Good call.

Also, should we allow the file to be added directly to the "files sidebar"?

No strong opinion, the user can always just skip the file action and this way we teach them it's an option?

Also, what about elixir and markdown files? Those don't need to be added to the files sidebar, as they can just become regular markdown or code cells. Thoughts?

Honestly, I don't think dropping whole elixir or markdown file into a single cell is particularly useful.

@josevalim
Copy link
Contributor

I see, though "Insert" generally is about putting something in the notebook content, we insert images, cells. If you insert image as markdown cell, it's not really processing, so I'm not really sure.

So "Embed file" is safer? Or maybe something also mentioning cells? as we are creating cells based on files? Or "File tasks" / "File suggestions" / "Cell suggestions from files"?

No strong opinion, the user can always just skip the file action and this way we teach them it's an option?

It is a bit contradictory because they have to choose a location, only to discard it. If supporting drag and drop into the files sidebar is not a big deal, then I would support it for convenience. :)


Agreed on the rest.

@jonatanklosko
Copy link
Member Author

"Suggested actions"?

It is a bit contradictory because they have to choose a location, only to discard it. If supporting drag and drop into the files sidebar is not a big deal, then I would support it for convenience. :)

Should we support dropping it anywhere in the notebook, or should we automatically expand sidebar and show an area there?

@josevalim
Copy link
Contributor

"Suggested actions"

Sounds good to me!

Should we support dropping it anywhere in the notebook, or should we automatically expand sidebar and show an area there?

Whatever works for you.

@jonatanklosko
Copy link
Member Author

Alright, I went with the additional area:

dnd_files.mp4

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.

LGTM! One last question: did you make the border of the DND (both in the sidebar and in the notebook) thicker and darker, compared to our usual borders, by design? If not, then we should probably align them. :)

@jonatanklosko
Copy link
Member Author

@josevalim yeah, it matches DND border in the other place we already had and it's both stronger and thicker :)

@jonatanklosko jonatanklosko merged commit 489b609 into main Jul 22, 2023
@jonatanklosko jonatanklosko deleted the jk-dnd branch July 22, 2023 09:13
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.

2 participants