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

More control over the prompt plugin #1916

Merged
merged 2 commits into from
Feb 7, 2020

Conversation

superruzafa
Copy link
Contributor

@superruzafa superruzafa commented Jan 30, 2020

This PR extends the idea introduced in #1597 which allowed the possibility of reuse the previous entered value if you marked Default to Last Value.

Background

I've found myself having a separate file with text strings to C&P them into the prompt dialogs because the behavior of that dialog wasn't as flexible as I wanted, so I tried to improve the current prompt dialog plugin with my own needs, but I thought they are general enough so they could be useful for others.

Feature

imagen

This PR leaves the user to choose between a couple of options about how to show the prompt:

  • on each request with an empty text box, ignoring any previously entered value (original behavior)
  • on each request but defaults to the prompt's default value, ignoring any previously entered value (new behavior)
  • on each request but defaults to the current stored value (aka Default to Last Value)
  • once and storing the value for futher requests (the behavior used when a Storage Key was set).

As a new feature now the prompt text is autoselected in order to quickly edit it if you want to change the value.

Remarks

In order to keep the backwards compatibility I transformed the current Default to Last Value booleans to the equivalents enums.

A broken behavior is that the prompt now is shown regardless a custom storage key has been set if the user choosed any of the on each request options, but this could be useful because until now there was no way to change the stored message unless one reload the whole application.

@nijikokun
Copy link
Contributor

I'm wondering if this should be split into it's own select, and retain the previous functionality as-is. Thoughts?

@superruzafa
Copy link
Contributor Author

superruzafa commented Feb 6, 2020

I choosed this approach because I think that the previous feature was a very specific case, while mine is more general and encompasses it (also it's backwards compatible)
Also I tried to prevent increasing the number of parameters of the prompt plugin.

This feature also allows the user to change the stored value without reload the whole app.

The label of the enum could be improved, though.

@gschier gschier self-requested a review February 7, 2020 16:04
Copy link
Contributor

@gschier gschier left a comment

Choose a reason for hiding this comment

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

I'm okay with this change. Thanks for submitting it @superruzafa! I might play around with the label names after merging as it seems a bit confusing.

@gschier gschier merged commit c7b0617 into Kong:develop Feb 7, 2020
@gschier
Copy link
Contributor

gschier commented Feb 7, 2020

@superruzafa After playing around with this, I can't actually find any new behavior that can't already be done with the old setup.

  • on each request with an empty text box, ignoring any previously entered value (original behavior)
    • Default to Last Value unchecked
    • Default empty
    • Storage Key unchecked
  • on each request but defaults to the prompt's default value, ignoring any previously entered value (new behavior)
    • Default to Last Value unchecked
    • Default set
    • Storage Key empty
  • on each request but defaults to the current stored value (aka Default to Last Value)
    • Default to Last Value checked
    • Default empty or set
    • Storage Key empty
  • once and storing the value for futher requests (the behavior used when a Storage Key was set).
    • Default to Last Value checked or unchecked
    • Default empty or set
    • Storage Key set

Going to keep the old behavior for now unless you can point out something I'm missing. I agree that having a dropdown does make it a bit nicer, but since we can't remove other params, it sends up complicating things too much.

gschier added a commit that referenced this pull request Feb 7, 2020
gschier added a commit that referenced this pull request Feb 7, 2020
@superruzafa
Copy link
Contributor Author

superruzafa commented Feb 8, 2020

@gschier, it seems that the behavior I though it couldn't be reproduced can be done in fact using a combination of settings. Having said that it's ok for me if you ignore this PR :)

But maybe we can learn something good after all. What about let the user to set the exactly behavior of the prompt by choosing an option from a dropdown instead of think what chekboxes/textboxes should be checked/unchecked/filled/emptied to have the same behavior?
That, I guess, is what this PR would be useful for.

Anyway, thanks a lot for your fantastic application!

@gschier
Copy link
Contributor

gschier commented Feb 10, 2020

I agree it would be useful to do that. However, I don't see an easy way to add this functionality without (1) duplicating options or (2) breaking backwards-compatibility. Perhaps a new prompt plugin v2 later on that implements all the things we've added since the beginning 😄

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.

None yet

3 participants