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

Added explanation of the fetch module. - #57

Open
teymour-aldridge wants to merge 16 commits into
yewstack:masterfrom
teymour-aldridge:explain-fetch-module
Open

Added explanation of the fetch module.#57
teymour-aldridge wants to merge 16 commits into
yewstack:masterfrom
teymour-aldridge:explain-fetch-module

Conversation

@teymour-aldridge

@teymour-aldridgeteymour-aldridge commented Apr 16, 2020

Copy link
Copy Markdown
Contributor

No description provided.

jstarry
jstarry reviewed Apr 19, 2020
View reviewed changes

@jstarryjstarry left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks for adding this!

Comment threadsrc/concepts/services/fetch.md Outdated
Comment threadsrc/concepts/services/fetch.md
Comment threadsrc/concepts/services/fetch.md
jstarry
jstarry reviewed May 2, 2020
View reviewed changes
Comment threadsrc/concepts/services/fetch.md Outdated
Comment threadsrc/concepts/services/fetch.md Outdated
if meta.status.is_success() {
match data.message {
"success" => {
Self::Message::ReceiveLocation(data.clone())

@jstarryjstarryMay 2, 2020

Copy link
Copy Markdown
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?

@teymour-aldridgeteymour-aldridgeMay 2, 2020

Copy link
Copy Markdown
ContributorAuthor

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 threadsrc/concepts/services/fetch.md Outdated
Comment on lines +88 to +90
if meta.status.is_success() {
match data.message {
"success" => {

@jstarryjstarryMay 2, 2020

Copy link
Copy Markdown
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"

@teymour-aldridgeteymour-aldridgeMay 2, 2020

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Yes.

@teymour-aldridgeteymour-aldridgeMay 2, 2020

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Done (I think)!

Comment threadsrc/concepts/services/fetch.md Outdated
Comment threadsrc/concepts/services/fetch.md Outdated
Comment threadsrc/concepts/services/fetch.md Outdated
Comment threadsrc/concepts/services/fetch.md Outdated
Comment threadsrc/concepts/services/fetch.md Outdated
fn view(&self) -> Html {
html! {
<>
{if self.fetching {

@jstarryjstarryMay 2, 2020

Copy link
Copy Markdown
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

@teymour-aldridgeteymour-aldridgeMay 2, 2020

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Done (I think)!

Comment threadsrc/concepts/services/fetch.md
teymour-aldridgeand others added 8 commits May 2, 2020 14:37
@teymour-aldridge@jstarry
Create FetchService on the fly.
d9ae58d
Co-authored-by: Justin Starry <justin.m.starry@gmail.com>
@teymour-aldridge@jstarry
Remove unnecessary fetch_service field.
38c39bd
Co-authored-by: Justin Starry <justin.m.starry@gmail.com>
@teymour-aldridge@jstarry
Remove redundant fetch_service field.
6a56bf8
Co-authored-by: Justin Starry <justin.m.starry@gmail.com>
@teymour-aldridge@jstarry
Update src/concepts/services/fetch.md
9e454be
Co-authored-by: Justin Starry <justin.m.starry@gmail.com>
@teymour-aldridge@jstarry
Update src/concepts/services/fetch.md
cb60e6d
Co-authored-by: Justin Starry <justin.m.starry@gmail.com>
@teymour-aldridge
Break out logic from view function.
43a6755
@teymour-aldridge
Update fetch.md
5f4d130
@teymour-aldridge
Add missing match expression in example.
aa5d513
jstarry
jstarry reviewed May 3, 2020
View reviewed changes
Comment threadsrc/concepts/services/fetch.md
// 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 {

@jstarryjstarryMay 3, 2020

Copy link
Copy Markdown
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?

@teymour-aldridgeteymour-aldridgeMay 8, 2020

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Fixed this (I think).

Comment threadsrc/concepts/services/fetch.md Outdated
ReceiveLocation(location) => {
self.iss = location;
self.fetching = false;
// we want to redraw so that the page no longer says 'fetching...'

@jstarryjstarryMay 3, 2020

Copy link
Copy Markdown
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!

Comment threadsrc/concepts/services/fetch.md
link: ComponentLink<Self>
}

impl FetchServiceExample {

@jstarryjstarryMay 3, 2020

Copy link
Copy Markdown
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

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

Comment threadsrc/concepts/services/fetch.md
Comment on lines +113 to +114
// handle errors more properly than this
Err(_) => panic!("Could not handle this error")

@jstarryjstarryMay 3, 2020

Copy link
Copy Markdown
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

@teymour-aldridgeteymour-aldridgeMay 3, 2020

Copy link
Copy Markdown
ContributorAuthor

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 freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@jstarryjstarryAwaiting requested review from jstarry

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@teymour-aldridge@jstarry