-
Notifications
You must be signed in to change notification settings - Fork 45
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
[RFC][Feature] Add ability to open modal using triggerID from command. #21
[RFC][Feature] Add ability to open modal using triggerID from command. #21
Conversation
const formFromPayload = (payload: any) => { | ||
if (payload.type === "view_submission") { | ||
const form = Object.keys(payload.view.state.values) | ||
.map((key) => [key, Object.keys(payload.view.state.values[key])[0]]) |
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.
Awesome job, I would like to have this merged as well. Just a pinpoint, nothing that really matter:
That could be simplified, without the need for 2 map
calls.
You can just build action
on the fly inside the 2nd map, along with data
const action = Object.keys(payload.view.state.values[key])[0]
Thank you @SirensOfTitan for your pull request, this is super cool! If the developer opens a modal from within a message (using const openModal = useModal(
"modal",
MyModal,
() => console.log("submit"),
() => console.log("cancel")
); and they also specify <Modal
onSubmit={() => console.log("submit")}
onCancel={() => console.log("cancel")}
title="A fancy pants modal"
submit="submit the form"
/> will both prop callbacks and the callbacks passed into |
Yeah, they'll both be executed right now. I like the idea of keeping track of where the modal was initiated! Perhaps with some typing alongside to make it clear which modality you're opting into? type ModalProps = {
/** Array of Actions, Context, Divider, ImageBlock, Input, or Section components */
children: PheliaChildren;
/** The title of the modal. */
title: ReactElement | string;
/**
* An optional plain_text Text component that defines the text displayed in the submit button
* at the bottom-right of the view. submit is required when an input block is within the
* blocks array. Max length of 24 characters.
*/
submit?: ReactElement | string;
} & ({
type: "inline"
} | {
type: "root",
onSubmit: ...
onCancel ...
}) If possible this kind of paradigm also might be nice (we use this pattern a lot in our react codebase): const openModal = useModal("modal1", () =>
<Modal onSubmit={() => ...} onCancel={() => ...})
);
// or
const openModal = useModal(() =>
<Modal key="modal1" onSubmit={() => ...} onCancel={() => ...} />
); |
Alright, I've added types differentiating between I'll loop back and test the additional code later this afternoon.
Thank you! I'm going to avoid adding anything to that part now, as it was all just moved code, but happy to push a PR for that after this is merged. |
Tested basic functionality against slack with the updates, looks good! |
@maxchehab done! |
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.
Perfect, thank you!
Released in |
This diff adds the ability to open up a modal using a
trigger_id
passed from some action like a command from inside slack.I do so by adding two new optional props to the
Modal
component:onSubmit
andonCancel
that are run on submission of a modal. If the model was triggered by auseModal
function, thenonSubmit
andonCancel
will run in both theModal
component and theuseModal
callbacks.I'm not really sure if this is the correct approach, it felt the most natural after a little discovery in the reconciler. Looking for feedback on method here, some discussion as to whether the author would like to merge this (I see this and ephemeral messages as the two small missing features), and how I might be able to adjust to merge.
I've tested the basic functionality in an application I'm building, but have done little verification past that.