-
Notifications
You must be signed in to change notification settings - Fork 28
Remove on-close events from json-config-editor #343
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
base: master
Are you sure you want to change the base?
Remove on-close events from json-config-editor #343
Conversation
efc74e3 to
045d887
Compare
|
Looks good generally. Once minor comment to address and I'll approve. |
| ); | ||
|
|
||
| if (submit === 'Yes') { | ||
| if (this.userClickedSubmit) { |
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.
The implementation works as described. However, I wonder if we should open a confirmation dialog before closing the config editor so that the user can cancel to not lose the configuration accidentally? The implementation could be similar to when closing a code editor with changes that have not been saved. WDYT @williamsyang-work ?
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.
During the original implementation of the customization UI I tried implementing that and wasn't able to. I think that stopping the closing of a file is one of those things that vscode reserves for itself and doesn't give extension developers. But I would need to verify that.
There are some other bugs we've found that I think should be addressed first. We can scope this in and attempt an implementation later?
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.
Ok. Thanks for the explanation. Too bad that the vscode APIs seem to be too restrictive. Still worth investigating a bit more to see if we manage to achieve this. We can scope this in later after your other bugfixes.
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 tested the config editor feature a bit more and noticed a few UX issues (without your PR). For example,
- when creating a config, the editor opens. The editor content is not saved at that moment. The name in header of the config editor is cursive. When I open a file from the file explorer, the config editor content is replaced and I get the submit configuration toast message is shown. With your PR, the editor for config would just be gone.
- when creating a config, the editor opens. Now I change the config. The editor content is now saved in a temporary file and the name in the header of the config editor is not cursive indicating a permanent opened editor. When I open a file from the file explorer, the new file opens and the name of the config editor is strikethrough (I guess the temporary file is deleted?) Then the submit configuration toast message is shown. With your PR, the editor for config would just be gone.
Both cases need some UX improvements. Are those cases part of the bugfixes that you're working on?
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 think these UX issues stem from #334.
dd9ea97 to
4305b21
Compare
This will force the user to use the button commands in the nav bar. Signed-off-by: Will Yang <william.yang@ericsson.com>
4305b21 to
8543ef3
Compare
What it does
Removes the prompt for config submission if the user closes the file. We found it was counter-intuitive UI. This will force the user to use the button commands in the nav bar.
How to test
Close the custom analysis json config file. No prompts should show up!
Follow-ups
Really just continued UI/UX improvements for custom analysis.
Review checklist