-
Notifications
You must be signed in to change notification settings - Fork 15
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
feat: improved error communication on frontend, with toast component #12
Conversation
@ShubhranshuSanjeev can you add a few screenshots? |
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.
Looks great! I've suggested a few changes and had a few questions
crates/frontend/src/types.rs
Outdated
@@ -214,3 +214,10 @@ pub struct BreadCrums { | |||
pub value: Option<String>, | |||
pub is_link: bool, | |||
} | |||
|
|||
/************************************************************************/ |
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.
We don't need this separation comment
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.
@ShubhranshuSanjeev can you remove this
crates/frontend/src/utils.rs
Outdated
url: String, | ||
method: reqwest::Method, | ||
body: Option<T>, | ||
headers: &[(&str, &str)], |
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.
It maybe better to take Headers
type directly instead of constructing it, just a thought
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.
If you see, you have to use HeaderName::from_str
and HeaderValue::from_str
on &str
for header name and value, respectively, these functions then return a Result
, which has to be handled to extract out the parsed value. Its a repeated procedure which has to be done for every request, seemed better to abstract it out.
Now that I am thinking about this, may be I can make another helper fxn to just construct the HeaderMap
which I can then pass to the request
.
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.
done
@@ -343,3 +349,66 @@ pub fn get_config_value( | |||
} | |||
} | |||
} | |||
|
|||
/********* Request Utils **********/ |
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.
We don't need these comments
crates/frontend/src/utils.rs
Outdated
.map_or(String::from("Something went wrong"), |error| error.message); | ||
enqueue_alert(error_msg.clone(), AlertType::Error, 3000); | ||
|
||
return Err(anyhow!(error_msg)); |
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.
Why anyhow?
crates/frontend/src/utils.rs
Outdated
|
||
response.json::<R>().await.map_err(|err| { | ||
logging::log!("{:?}", err); | ||
enqueue_alert(err.to_string(), AlertType::Error, 2000); |
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.
What does enqueue_alert
do here?
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.
Ignore, it is for showing the alert on the UI.
|
||
#[derive(Clone, Debug)] | ||
pub struct Alert { | ||
pub id: u64, |
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.
is ID required?
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 need it for removing the correct alert after the timeout.
} | ||
|
||
impl Alert { | ||
pub fn default(id: u64, text: String) -> Self { |
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.
Can we auto generate the ID in default?
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.
If you check the id is generated in enqueue_alert
fxn, you won't have to give a id of your own most of the time, as for general use-case you can just fire the enqueue_alert
.
Use request fn for the function's utils also. |
let outer_div_class = format!("alert {}", alert.alert_type.to_css_class()); | ||
let content_icon = alert.alert_type.to_icon_class(); | ||
view! { | ||
<div class="toast toast-end"> |
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.
provide z-index greater than the drawer.
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.
Done
id, | ||
text, | ||
alert_type: AlertType::Info, | ||
timeout: 500, |
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.
Increase the timeout.
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.
Done
I think we should me the error from the Api's a bit more concise as it sometimes gives nearly a paragraph, which doesn't look good in the toast. |
@ShubhranshuSanjeev Is success toast going to be a part of another PR? |
let mut request_builder = | ||
HTTP_CLIENT.request(method.clone(), url).headers(header_map); | ||
request_builder = match (method, body) { | ||
(reqwest::Method::GET | reqwest::Method::DELETE, _) => request_builder, |
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.
Import reqwest::Method above.
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.
Don't really think we need to import, this is good enough.
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.
importing this would make this more concise and cleaner, not mandatory, just a recommendation.
@@ -36,6 +39,23 @@ pub fn Layout(children: Children) -> impl IntoView { | |||
<main class="ease-soft-in-out xl:ml-96 relative h-full max-h-screen rounded-xl transition-all duration-200 overflow-y-auto"> | |||
{children()} | |||
</main> | |||
|
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.
make a module for this.
@mahatoankitkumar , its already there, you can make success toasts too, you just have to that in |
@mahatoankitkumar , will do that separately, not really in scope of this PR. |
I think this should be a part of this PR itself, doesn't seems to be a major change. |
Oh, got it what you mean, you are asking me to show success toasts on successful API calls, right? |
Yup |
3b0cbb9
to
a2c8133
Compare
a2c8133
to
cd0ff47
Compare
@mahatoankitkumar , made changes for this |
a8b2ac4
to
aa8cbce
Compare
@ShubhranshuSanjeev There seems to be an issue in the initial load. After that it starts working fine. Screen.Recording.2024-05-02.at.5.03.03.PM.mov |
"schema validation failed for {key} with error {:?}", | ||
verrors | ||
"schema validation failed for {key}: {}", | ||
validation_err_to_str(verrors).first().unwrap() |
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.
we can do unwrap_or(verrors)
as a fallback.
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.
Not needed I guess, there will always be one item in the vector
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.
Anyway its a str, i will change this to unwrap_or(String::new())
.await | ||
.map_or(String::from("Something went wrong"), |error| error.message); | ||
enqueue_alert(error_msg.clone(), AlertType::Error, 5000); | ||
|
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.
log the error here.
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
.get() | ||
.map_or( | ||
String::new(), | ||
|o| { format!("{}\n{}", o.message, o.stdout) }, |
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.
Nice, was doing it myself, just a request if you can do the same for the above error_message.get().
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.
Sure will do it
&default_config | ||
.get_value() | ||
.into_iter() | ||
.map(ConfigType::DefaultConfig) |
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.
This is weirdly formatted
crates/frontend/src/types.rs
Outdated
@@ -214,3 +214,10 @@ pub struct BreadCrums { | |||
pub value: Option<String>, | |||
pub is_link: bool, | |||
} | |||
|
|||
/************************************************************************/ |
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.
@ShubhranshuSanjeev can you remove this
aa8cbce
to
c229fcc
Compare
Something weird happening in Leptos, have to understand what's going on. |
c229fcc
to
2ec95f5
Compare
2ec95f5
to
d15d269
Compare
d15d269
to
26c8004
Compare
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.
Initial load issue to be fixed.
Problem
Previously, our application lacked proper user feedback mechanisms when dealing with API failures or form submissions. There was no standardized way to communicate error messages to the user.
Solution
AlertProvider
context that manages a queue of alerts to be displayed on the screen. This context allows for centralized handling of alert messages throughout the application.request
that can be directly used for making API calls. This function automatically inserts any errors that occur during the API call into the alert queue, ensuring that users are promptly notified of any issues.UI Screenshot