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

Fix errors when saving a blank hotkey #2430

Merged
merged 6 commits into from
Nov 6, 2018
Merged

Fix errors when saving a blank hotkey #2430

merged 6 commits into from
Nov 6, 2018

Conversation

ghost
Copy link

@ghost ghost commented Sep 29, 2018

Saving a blank hotkey showed false error even though hotkey was
saved. This solution solves the issue as it stands, but I
would recommend looking into cleaning up the error handling
surrounding this functionality before adding any new hotkey
fields.

Refs #2153

@ZeroX-DG ZeroX-DG added the awaiting review ❇️ Pull request is awaiting a review. label Sep 29, 2018
@ghost
Copy link
Author

ghost commented Sep 29, 2018

This also satisfies #2087, I think.

@ZeroX-DG
Copy link
Member

Hey, @samherrington thank you for your contribution 🎉
Please improve your code because with the current code the error won't appear if the Show/Hide Boostnote hotkey is empty, however, it also doesn't display a success message when the user save the setting with the empty Show/Hide Boostnote hotkey

boostnote_bug_hotkey

@ZeroX-DG ZeroX-DG added awaiting changes 🖊️ Pull request has been reviewed, but contributor needs to make changes. and removed awaiting review ❇️ Pull request is awaiting a review. labels Sep 30, 2018
samherrington and others added 6 commits October 1, 2018 09:24
Saving a blank hotkey showed false error even though hotkey was
saved. This solution solves the issue as it stands, but I
would recommend looking into cleaning up the error handling
surrounding this functionality before adding any new hotkey
fields.
Saving a blank hotkey will now display a success message when the user saves the setting with the empty Show/Hide Boostnote hotkey.
@jacobherrington
Copy link
Contributor

@ZeroX-DG I am fairly sure the error messages correctly reflect what is happening now. If the success message shows up, the hotkey seems to be consistently set to what is in the text box.

@ZeroX-DG
Copy link
Member

ZeroX-DG commented Oct 9, 2018

Well, clearly that's the current behavior 😄 Or we will have to prevent users from saving blank hotkeys, however, there're requests that people want to "disable" that hotkey by leaving it blank. So do you have other ideas other than sticking to the current behavior? If not then I think showing a message to notify the user is not a bad idea.

@ghost
Copy link
Author

ghost commented Oct 9, 2018

@ZeroX-DG The new behavior allows users to save blank hotkeys and displays the success message when they do (because they have successfully saved a blank hotkey). Before this fix, a blank hotkey returned an error.

@ZeroX-DG
Copy link
Member

boostnote_bug_hotkey
@samherrington is this the intended behavior?

@ghost
Copy link
Author

ghost commented Oct 14, 2018

@ZeroX-DG No, with this PR you should see "Successfully applied!" when either or both of the hotkeys are blank.

Copy link
Member

@ZeroX-DG ZeroX-DG left a comment

Choose a reason for hiding this comment

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

Sorry @samherrington, it's was a bug in vscode pull request extension. How can you submit the PR when @jacobherrington own the repo? This is new to me 😄 Anyway LGTM 🎉

@ZeroX-DG ZeroX-DG added approved 👍 Pull request has been approved by sufficient reviewers. and removed awaiting changes 🖊️ Pull request has been reviewed, but contributor needs to make changes. labels Oct 14, 2018
@jacobherrington
Copy link
Contributor

@ZeroX-DG He is a collaborator on my fork!

@ZeroX-DG
Copy link
Member

Haha @jacobherrington TIL

@Rokt33r Rokt33r added next release (v0.11.12) and removed approved 👍 Pull request has been approved by sufficient reviewers. labels Nov 6, 2018
@Rokt33r Rokt33r merged commit 27e5010 into BoostIO:master Nov 6, 2018
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