Skip to content
This repository has been archived by the owner on Jul 19, 2020. It is now read-only.

Added explanation of the fetch module. #57

Open
wants to merge 16 commits into
base: master
Choose a base branch
from

Conversation

teymour-aldridge
Copy link
Contributor

No description provided.

Copy link
Member

@jstarry jstarry left a comment

Choose a reason for hiding this comment

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

Thanks for adding this!

src/concepts/services/fetch.md Outdated Show resolved Hide resolved
src/concepts/services/fetch.md Show resolved Hide resolved
src/concepts/services/fetch.md Show resolved Hide resolved
src/concepts/services/fetch.md Outdated Show resolved Hide resolved
if meta.status.is_success() {
match data.message {
"success" => {
Self::Message::ReceiveLocation(data.clone())
Copy link
Member

Choose a reason for hiding this comment

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

Is this clone needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure, I think not.

Comment on lines 88 to 90
if meta.status.is_success() {
match data.message {
"success" => {
Copy link
Member

Choose a reason for hiding this comment

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

I think we should be more succinct here: if meta.status.is_success() && data.message == "success"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done (I think)!

src/concepts/services/fetch.md Outdated Show resolved Hide resolved
src/concepts/services/fetch.md Outdated Show resolved Hide resolved
src/concepts/services/fetch.md Outdated Show resolved Hide resolved
src/concepts/services/fetch.md Outdated Show resolved Hide resolved
fn view(&self) -> Html {
html! {
<>
{if self.fetching {
Copy link
Member

Choose a reason for hiding this comment

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

Can you please break out this block and the match self.iss block into their own methods on FetchServiceExample? I'd like the examples to set a good precedent for splitting up view logic

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done (I think)!

src/concepts/services/fetch.md Show resolved Hide resolved
// split up the response into the HTTP data about the request result and data from the request
let (meta, Json(data)) = response.into_parts();
if meta.status.is_success() && data.message == "success" {
Self::Message::ReceiveLocation(match data {
Copy link
Member

Choose a reason for hiding this comment

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

What type is data?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed this (I think).

ReceiveLocation(location) => {
self.iss = location;
self.fetching = false;
// we want to redraw so that the page no longer says 'fetching...'
Copy link
Member

Choose a reason for hiding this comment

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

nit: and also display the location!

link: ComponentLink<Self>
}

impl FetchServiceExample {
Copy link
Member

Choose a reason for hiding this comment

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

nit: can you move this impl block under the impl Component block? It flows better because from top to bottom you see the methods right after seeing

    fn view(&self) -> Html {
        html! {
            <>
                {self.is_fetching()}
                {self.view_iss_location()}
            </>
        }
    }

Comment on lines +113 to +114
// handle errors more properly than this
Err(_) => panic!("Could not handle this error")
Copy link
Member

Choose a reason for hiding this comment

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

Maybe we should handle the error to show a more complete example? Not sure if people get tripped up on error handling or not

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll add some error handling.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants