-
Notifications
You must be signed in to change notification settings - Fork 1
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
alerts #20
alerts #20
Conversation
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.
❌ Changes requested. Reviewed everything up to 110484e in 2 minutes and 47 seconds
More details
- Looked at
75
lines of code in1
files - Skipped
0
files when reviewing. - Skipped posting
1
drafted comments based on config settings.
1. app.js:105
- Draft comment:
The error message in the save method should be specific to the action being performed. Consider updating it to reflect the need for login.
Swal.fire({
icon: "warning",
title: "Login Required",
text: "Please login to save files."
});
- Reason this comment was not posted:
Confidence of 0% on close inspection, compared to threshold of 50%.
Workflow ID: wflow_Pcxzi8S8UZq51Aqa
Want Ellipsis to fix these issues? Tag @ellipsis-dev
in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet
mode, and more.
⌛ 5 days left in your free trial, upgrade for $20/seat/month or contact us.
title: "Logged in!", | ||
text: "Logged in with public key!", | ||
title: "Authenticated!", | ||
text: "File loaded to pastebin!", |
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 success message 'File loaded to pastebin!' is misleading as it appears after a successful login, not after a file load. Consider reverting to the original or a more appropriate message.
text: "File loaded to pastebin!", | |
text: "Logged in with public key!", |
@@ -85,7 +85,7 @@ export class App extends Component { | |||
// Load the file content from the server | |||
loadFile = async () => { | |||
if (!this.state.userPublicKey) { | |||
alert('Please login first'); | |||
// alert('Please login first'); |
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 original alert 'Please login first' in the loadFile method is commented out without a replacement, potentially leading to a confusing user experience. Consider adding a user feedback mechanism here.
// alert('Please login first'); | |
Swal.fire({ | |
icon: "warning", | |
title: "Login Required", | |
text: "Please login to load files." | |
}); |
icon: "success", | ||
timer: 1500 | ||
}); | ||
} | ||
|
||
if (!success) { |
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 error handling in the save method is redundant. Consider removing the redundant check and handling all errors in the catch block.
if (!success) { | |
// This block can be removed if saveFile throws an exception on failure |
Summary:
Enhanced user interaction in
app.js
by replacing alerts with SweetAlerts and adding robust error handling in file operations.Key points:
userLogin
andsave
methods inApp
class inapp.js
to useSwal.fire
instead ofalert
.save
method with try-catch block.Generated with ❤️ by ellipsis.dev