bruno
bruno copied to clipboard
usebruno
Prevent losing unsaved environment variable data when attempting to change env
Description
Addressing https://github.com/usebruno/bruno/issues/1997
When modifying environment variables, changing environments without clicking the "Save" button loses the user's progress. This can be frustrating as there are no warnings beforehand.
With this pull request, I fixed this bug by detecting if any field in a variable in the current environment is changed, or if environment variables were added or deleted. If so, it gives a warning and prevents the user from switching, creating, and improving environments without either first saving or reverting their changes. Their changes will not be unexpectedly lost.
https://github.com/usebruno/bruno/assets/86133477/ceba5a4a-16a2-4377-81fe-2155cde9bcb2
Contribution Checklist:
- [X] The pull request only addresses one issue or adds one feature.
- [X] The pull request does not introduce any breaking changes
- [X] I have added screenshots or gifs to help explain the change if applicable.
- [X] I have read the contribution guidelines.
- [x] Create an issue and link to the pull request.
Note: Keeping the PR small and focused helps make it easier to review and merge. If you have multiple changes you want to make, please consider submitting them as separate pull requests.
Publishing to New Package Managers
Please see here for more information.
@AngelaSYuan I would argue that might not be great UX. For exaple, what if you don't care about the change and decide to simply discard it? You now HAVE to save. You did not show if you could close the dialog and discard the changes that way.
There is already an existing dialog that IMO has better UX and gives the user a choice:
Can you implement that dialog?
@AngelaSYuan You can just pay the $19 to support the project lol its not like you wont use it. But I have no control over licensing, maybe @sanjai0py can get you a license key.
@VictorioBerra True haha, thanks! I'm going to work on implementing this dialog for the environment switch.
I just added the functionalities for the dialog box! The PR should be updated @VictorioBerra @sanjai0py
https://github.com/usebruno/bruno/assets/86133477/404881b4-d385-4c76-82d1-301549b457d7
@AngelaSYuan I like it, hopefully they approve. Slight nitpick but is there a way to make other dialogs not open after it? For example when you clicked "+ Create" it prompted you with the save dialog, and when you dismissed it (either with dont save, cancel or save) it immediately threw up the new environment dialog. Is there a way to make it not do that?
Like i said nitpick I dont think that should make or break this PR since this is a big life saver we did not have before.
@VictorioBerra Thanks! And got it, are you saying when I clicked on "+ Create", after I hit "Save", the "Create Environment" dialog popped up immediately after? I had intended that to be a feature haha, so that the user didn't need to click on "+ Create" again after clicking save on the warning dialog.
But I can definitely have the screen return to just showing the variables (no immediately following dialog), if that's what you mean? I can see how it might not be UX friendly in some cases.
@AngelaSYuan Yeah I think it can be jarring to be bombarded with dialogs. I think just return the user to where they were before they tried to leave without saving. Obviously if they click "Dont Save" I would expect the changes to be reverted.
Just fixed, so that we don't immediately switch environments/pop up the other dialog, whether we click "Don't Save" or "Save." Let me know if you have any other suggestions/if it might be able to be merged! @sanjai0py @VictorioBerra
https://github.com/usebruno/bruno/assets/86133477/feca6d2d-86b6-460a-8556-b347432e1ca7
@Its-treason Thank you! I just fixed the confirm warning modal to be centered using CreatePortal, and got rid of the isModified state. I also removed comments and unused imports. Let me know how it looks!
@sanjai0py @helloanoop Hi, just curious if my PR is merge-ready? If not, what other steps do I need to take?
Hi @AngelaSYuan !
Thanks for working on this. I am onboard with the UX updates done.
I am worried about the formik logic moved to 2 levels up in the component tree.
Ideally, we would want the formik stuff to stay inside the component that renders the form.
Was this intentional or did you have to move it because you were not able to the make the close dialog stuff work with the formik staying inside the EnvironmentVariables component ?
Hi @helloanoop, thank you! I would say it was both intentional in terms of the design, but also so that I could pass down the appropriate variables for the close dialog functionality (which wasn't possible with formik being in the EnvironmentVariables component.
Would this change be alright? I believe everything should work the same since these components are rendered together, and moving the formik would enable us to access the needed variables!
Hi @helloanoop! I just wanted to check in and see if this modification would be alright? For context, my school assignment on getting a PR merged with Bruno is due this Thursday, so I wanted to see if it's still possible to merge before then. I really appreciate your time!
Hi @AngelaSYuan
We need to figure out how this can be made possible with formik being in the EnvironmentVariables component.
I understand that the react way of passing data down the component tree makes this pattern difficult to implement, but we need to figure out a way.
One way to implement this might be to dispatch an action whenever an environment gets edited and add a property called 'dirty' as true inside the environment.
You can then use the dirty flag check before switching the environment
@helloanoop Got it! I just made changes to the PR. Now, formik is back in EnvironmentVariables and I use a dirty flag to track if the field has been modified. How does this update look to you?