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 prompt values to be clear + add actions to template tag plugin API #2736

Merged

Conversation

jgiovaresco
Copy link
Contributor

@jgiovaresco jgiovaresco commented Oct 17, 2020

Closes #1456

Implementation of what Greg describes in this comment: add a way to define actions in template tag.

Some screenshot of the TagEditor

With 1 action
Screenshot 2020-11-27 at 09 12 14

With more actions
Screenshot 2020-10-17 at 18 47 24
image

Clear prompt values
clear-prompt-value

@develohpanda
Copy link
Contributor

develohpanda commented Oct 20, 2020

However, I'm blocking on how we can provide some context to the action callbacks.

My guess is to investigate how the helperContext is built (when needed) for running the template tag, and do something similar for the button, in base-extension. Note that the context should be built when required. For example, once the button is clicked, generate the context and then send it to the plugin.

result = this.run(helperContext, ...args);

I suggest only exposing the store context for now, but it should be built in the standard way that allows other contexts' to be added, should they be required.

@jgiovaresco jgiovaresco force-pushed the fix/1456-allow-prompt-values-to-be-clear branch from a6bc124 to f8d82c9 Compare October 21, 2020 18:16
@jgiovaresco jgiovaresco force-pushed the fix/1456-allow-prompt-values-to-be-clear branch from f8d82c9 to 3a79b8a Compare October 22, 2020 05:45
@jgiovaresco
Copy link
Contributor Author

@develohpanda

I've managed to provide a context to the action callback. I had to look deeper on how plugins & nunjunks tags are working but it was interesting :)
I'm wondering if I should create a PR that will add only the feature "Define actions buttons on a template tag" and another that will fix the original issue? Or I keep this one 🤔

@jgiovaresco jgiovaresco marked this pull request as ready for review October 22, 2020 05:45
Copy link
Contributor

@develohpanda develohpanda left a comment

Choose a reason for hiding this comment

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

I've managed to provide a context to the action callback. I had to look deeper on how plugins & nunjunks tags are working but it was interesting :)

💯

I'm wondering if I should create a PR that will add only the feature "Define actions buttons on a template tag" and another that will fix the original issue? Or I keep this one 🤔

Am I understanding correctly that in order to create another PR to fix the original issue, you will only need to move the changes in plugins/insomnia-plugin-prompt/index.js? If that's the case, I think what you've done here is fine. In order to fix the original issue, the plugin API needed to be extended so this is all related.

Sorry about the delay in getting to this, I've been really bogged down!

One request: could you please create new commits when addressing review comments, rather than amending existing commits? It's difficult to see what has changed between iterations, and requires us to review the entire PR again. It can become quite time consuming with even a moderately sized PR. Because we squash and merge, a clean commit history in a PR is not so important for us. Thank you! 🤗

@develohpanda develohpanda changed the title Allow prompt values to be clear Allow prompt values to be clear + add actions to template tag plugin API Nov 27, 2020
@netlify
Copy link

netlify bot commented Nov 27, 2020

Deploy preview for insomnia-storybook ready!

Built with commit 5b7e574

https://deploy-preview-2736--insomnia-storybook.netlify.app

Copy link
Contributor

@develohpanda develohpanda left a comment

Choose a reason for hiding this comment

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

LGTM 👍

Copy link
Contributor

@DMarby DMarby left a comment

Choose a reason for hiding this comment

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

Looks good other then the mismatching type definition 👍

@develohpanda develohpanda merged commit dfc89a3 into Kong:develop Nov 30, 2020
@develohpanda
Copy link
Contributor

@smacktoid
Copy link

Amazing! My issue got fixed!

@develohpanda
Copy link
Contributor

Thanks for reporting it initially, @scottmmmm! We do have a release planned for this week, unfortunately this PR won't make it in. There will likely be a patch release early 2021 with this included!

@jgiovaresco jgiovaresco deleted the fix/1456-allow-prompt-values-to-be-clear branch December 2, 2020 16:43
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.

[Feature Request] Allow prompt values to be cleared
4 participants