-
Notifications
You must be signed in to change notification settings - Fork 423
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 Form to "+ Block" #1750
Add Form to "+ Block" #1750
Conversation
Uffizzi Preview |
Hi @ByeongUkChoi! I realized I made a mistake. Actually, we don't want to push people towards using these inputs because they won't work with apps. Instead, we should push them towards forms, so I think we can do some small changes:
form =
Kino.Control.form(
[
name: Kino.Input.text("Name"),
],
submit: "Submit"
)
form
|> Kino.render()
|> Kino.Control.stream()
|> Kino.listen(fn event ->
IO.inspect(event)
end) Apologies for the back and forth. What do you think @jonatanklosko? Also, should we make Kino.Input and Kino.Control enumerables by default, so we can skip the conversion in
The only issue is that there is some ambiguity if you want to listen to an actual list, but then I don't see the point and we can raise if it is not a list of event sources. |
PR for simplifying listen here: https://github.com/livebook-dev/kino/pull/261/files |
It very much depends on the use case, we shouldn't be pushing towards either because it's all up to what they want to do 🤔
There's also |
@jonatanklosko at the moment, forms have more use in apps, so let’s push towards that first. We can add inputs once runbooks are a thing. let’s discuss the control/input stuff in the kino PR? :) |
@ByeongUkChoi this looks great. Just one note: you are asking to install the Kino package, even when it is already installed. Can we address this somehow? Thank you! ❤️ |
d926368
to
67971c3
Compare
67971c3
to
899d7a7
Compare
f909b1e
to
33bc7e2
Compare
@josevalim @jonatanklosko |
Ah this is tricky, for smart cells, once packages are installed the runtime reports installed smart cells and indicates they have no "requirements" (which overrides the initial definition with "requirements"). I think we will need to pass input/snippet definitions to the runtime and let it filter out the requirements that are satisfied. If you want to take a stab at this, you look at how we configure smart cell definitions:
If you prefer me to have a look, I can do that later, it's fine either way! |
@jonatanklosko 1. form definition put in @extra_smart_cell_definitions (Livebook.Runtime.ElixirStandalone). like this: @extra_smart_cell_definitions [
...
%{
kind: "Elixir.Kino.Form",
name: "Form",
requirement: %{
variants: [
%{name: "Default", packages: [kino]}
]
}
}
] but this case. not found definitions # Livebook.Runtime.ErlDist.RuntimeServer.report_smart_cell_definitions/1
smart_cell_definitions = get_smart_cell_definitions(state.smart_cell_definitions_module)
# [] So I can't replace to 2. Save the
|
@ByeongUkChoi the flow is going to be similar to smart cells, but it needs to be separate, like |
@jonatanklosko |
33bc7e2
to
96401af
Compare
@ByeongUkChoi I think need the runtime (and hence the "Setup runtime" modal), otherwise we don't know if the dependency is installed. |
@jonatanklosko So when open "Setup runtime" modal? For smart cells, when I press the "+Smart" button, a modal pops up. |
We should have "+ Input", and if the runtime is not started then clicking on it will show "Setup runtime", otherwise it will have a sub-menu with the available inputs, just as we have for smart cells :) |
Sorry, there is some confusion. We gave up on |
@josevalim but we want to list multiple variants, or just the text one? |
@jonatanklosko just the text one for now, I think we only need something to get started and people will build the rest. |
Ah I see. In that case I think we can do either:
|
@jonatanklosko why can't we always show the "Form" button? Is it because we cannot always add Kino? Maybe we can just ask the runtime if it can install dependencies instead? That should be enough, no? |
@josevalim if we want to show the modal saying "Requires :kino, do you want to add it and restart?" (only if kino is missing), then we need to know whether kino is added. Alternatively we could analyze the setup cell to check if the dependency is there. It's a bit less reliable, because if there is Sidenote: in the future we could add a kino API for registering custom snippets, just something to keep in mind. |
@ByeongUkChoi sorry for the back and forth, I discussed with Jose offline and we figured the desired behaviour. There are some tiny complexities and I think it's easier for me to finish, thanks for the prototype! :) |
@jonatanklosko Great! So How can I do? |
@ByeongUkChoi it's all good, what I meant is that I will finish the implementation :D |
form = | ||
Kino.Control.form( | ||
[ | ||
name: Kino.Input.text("Name") | ||
], | ||
submit: "Submit" | ||
) | ||
|
||
Kino.listen(form, fn event -> | ||
IO.inspect(event) | ||
end) | ||
|
||
form\ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@josevalim I updated the snippet to remove the explicit render, and this way we return just the form rather than :ok
, but feel free to change :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
with_confirm( | ||
JS.push("add_smart_cell_dependencies", | ||
value: %{kind: definition.kind, variant_idx: variant_idx} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I moved this confirm to the server too, this way they are more aligned. It also reduces the rendered payload at a cost of an additional roundtrip when they hit insert and we need to add deps (which sounds fair).
via. #1743
I've tried to write it roughly.
I would like to get some advice on putting
input_cells_definitions
inRuntime
.