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

MyLand refresh/navigation fix #333

Merged
merged 21 commits into from
Apr 24, 2019
Merged

Conversation

marcelomorgado
Copy link
Contributor

Issue link

#267

Auto-close the issue?

Closes #267

Types of changes

Bug fix (non-breaking change that fixes an issue)

New feature (non-breaking change that adds functionality)

Notes

Could you tell more about your last comment on the issue?

I think the pattern to follow here is that there's a redux state update for each network request when request is initiated, when it succeeds, and when it fails due to timeout or error.

@pcowgill
Copy link
Member

I think the pattern to follow here is that there's a redux state update for each network request when request is initiated, when it succeeds, and when it fails due to timeout or error.

Could you tell more about your last comment on the issue?

Sure! Each type of network request - A) fetching my land; B) fetching land for sale; C) fetching account funding plus approval should have a name for that network request, and then when it it starts we save to redux that network request with name BLA_BLA is now in one of the enum list of options for network request status, between 1) loading, 2) succeeded, 3) failed.

The relevant one here would be the my land request.

Although going too far down this path might not be the best use of time because of the React Suspense discussion here #197

Does that clarify it, or should I go into more detail?

@marcelomorgado
Copy link
Contributor Author

marcelomorgado commented Apr 22, 2019

I think the pattern to follow here is that there's a redux state update for each network request when request is initiated, when it succeeds, and when it fails due to timeout or error.

Could you tell more about your last comment on the issue?

Sure! Each type of network request - A) fetching my land; B) fetching land for sale; C) fetching account funding plus approval should have a name for that network request, and then when it it starts we save to redux that network request with name BLA_BLA is now in one of the enum list of options for network request status, between 1) loading, 2) succeeded, 3) failed.

The relevant one here would be the my land request.

Although going too far down this path might not be the best use of time because of the React Suspense discussion here #197

Does that clarify it, or should I go into more detail?

Oh, I see, something like this?

{ type: 'FETCH_POSTS_REQUEST' }
{ type: 'FETCH_POSTS_FAILURE', error: 'Oops' }
{ type: 'FETCH_POSTS_SUCCESS', response: { ... } }

Extracted from redux doc.

@pcowgill
Copy link
Member

Oh, I see, something like this:

{ type: 'FETCH_POSTS_REQUEST' }
{ type: 'FETCH_POSTS_FAILURE', error: 'Oops' }
{ type: 'FETCH_POSTS_SUCCESS', response: { ... } }

Extracted from redux doc.

Yep, exactly

@pcowgill
Copy link
Member

@marcelomorgado I know there were a couple issues being tracked in issue #267 . Have some of them since become obsolete? Just checking whether this PR indeed closes all of them.

@marcelomorgado
Copy link
Contributor Author

marcelomorgado commented Apr 22, 2019

@marcelomorgado I know there were a couple issues being tracked in issue #267 . Have some of them since become obsolete? Just checking whether this PR indeed closes all of them.

This PR is addressing:

  • Fix: Refreshing from blockchain was removed on-process purchase lands;
  • Fix: After redirect user to MyLandScreen the menu link Land For Sale was jumped the user to Buy Screen of already bought land.
  • UX improvement: Redirect the user to MyLandScreen after clicking on Buy button (only possible after fixing both bugs above).

Copy link
Member

@pcowgill pcowgill left a comment

Choose a reason for hiding this comment

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

@marcelomorgado Great work!

decentraland/helpers/index.js Outdated Show resolved Hide resolved
decentraland/redux/reducers.js Outdated Show resolved Hide resolved
decentraland/screens/BuyLandScreen.js Outdated Show resolved Hide resolved
decentraland/constants/MyAssetStatus.js Outdated Show resolved Hide resolved
decentraland/screens/BuyLandScreen.js Show resolved Hide resolved
decentraland/screens/BuyLandScreen.test.js Outdated Show resolved Hide resolved
decentraland/screens/MyAssetsScreen.js Outdated Show resolved Hide resolved
decentraland/screens/MyAssetsScreen.js Show resolved Hide resolved
@pcowgill
Copy link
Member

@marcelomorgado I know there were a couple issues being tracked in issue #267 . Have some of them since become obsolete? Just checking whether this PR indeed closes all of them.

This PR is addressing:

* Fix: Refreshing from blockchain was removed on-process purchase lands;

* Fix: After redirect user to `MyLandScreen` the menu link `Land For Sale` was jumped the user to Buy Screen of already bought land.

* UX improvement: Redirect the user to `MyLandScreen` after clicking on `Buy` button (only possible after fixing both bugs above).

Awesome, this is a big improvement!!

pcowgill
pcowgill previously approved these changes Apr 23, 2019
decentraland/redux/reducers.js Show resolved Hide resolved
decentraland/redux/reducers.js Outdated Show resolved Hide resolved
pcowgill
pcowgill previously approved these changes Apr 23, 2019
pcowgill
pcowgill previously approved these changes Apr 23, 2019
pcowgill
pcowgill previously approved these changes Apr 23, 2019
decentraland/screens/MyAssetsScreen.js Outdated Show resolved Hide resolved
pcowgill
pcowgill previously approved these changes Apr 23, 2019
Copy link
Member

@pcowgill pcowgill left a comment

Choose a reason for hiding this comment

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

@marcelomorgado This is looking great! It's very impressive how quickly you turned these changes around.

decentraland/App.js Show resolved Hide resolved
decentraland/App.js Show resolved Hide resolved
decentraland/constants/UserActionStatus.js Outdated Show resolved Hide resolved
decentraland/constants/UserActionStatus.js Show resolved Hide resolved
decentraland/helpers/storage.js Show resolved Hide resolved
decentraland/screens/MyAssetsScreen.js Show resolved Hide resolved
decentraland/screens/MyAssetsScreen.js Show resolved Hide resolved
@pcowgill pcowgill dismissed stale reviews from themself via 22799c5 April 24, 2019 01:35
@pcowgill pcowgill merged commit 0665819 into develop Apr 24, 2019
@pcowgill pcowgill deleted the feature/myassets-refresh-fix branch April 24, 2019 01:40
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.

After purchase navigation
2 participants