Removed duplicate loading screen #237

pcowgill · 2019-04-01T22:26:47Z

Issue link

#166
#217

Auto-close the issue?

Closes #166
Closes #217

Types of changes

New feature (non-breaking change that adds functionality)

Technical debt (a code change that doesn't fix a bug or add a feature but makes something clearer for devs)

pcowgill · 2019-04-01T22:31:15Z

@marcelomorgado This might be a WIP still, because right now after loading the app locally on this feature branch I'm just seeing an empty list of land for sale

marcelomorgado · 2019-04-02T12:32:11Z

@marcelomorgado This might be a WIP still, because right now after loading the app locally on this feature branch I'm just seeing an empty list of land for sale

Yeah, I've worked on that last week and not figured out why it's happening yet.

pcowgill · 2019-04-02T14:42:22Z

@marcelomorgado I tracked down the error:

pcowgill · 2019-04-02T14:47:57Z

@marcelomorgado So I think maybe after reworking the AppNavigator file we're not properly wrapping the app with Root for native-base to work?

pcowgill · 2019-04-02T15:08:12Z

@marcelomorgado Wrapping the ListLandForSaleScreen in Root from native-base addresses the issue, for the record. But that's not the most elegant solution, since using the Root from App.js should be able to work if we set it up right.

pcowgill · 2019-04-02T17:41:34Z

@marcelomorgado Thoughts on whether we should merge in its current form with the Root hack after removing console.logs, or try to fix that first?

marcelomorgado · 2019-04-02T18:04:12Z

Hey @pcowgill , I've found this comment wix/react-native-navigation#4640 (comment) event it isn't totally clear for me, seems that the Root component will not work from App (we can remove it). I've quickly tried to add it programmatically inside of AppNavigator but my attempts until now don't work. I think will be better do a comment about it and go ahead. Make sense?

marcelomorgado · 2019-04-02T18:10:54Z

Hey @pcowgill , I've found this comment wix/react-native-navigation#4640 (comment) event it isn't totally clear for me, seems that the Root component will not work from App (we can remove it). I've quickly tried to add it programmatically inside of AppNavigator but my attempts until now don't work. I think will be better do a comment about it and go ahead. Make sense?

If you agree with me, this PR is ready to merge.

pcowgill · 2019-04-02T18:36:37Z

decentraland/App.js

@@ -11,7 +11,6 @@ import { Action } from "tasit-sdk";
 const { ConfigLoader } = Action;
 import tasitSdkConfig from "./config/default";
 import { checkBlockchain, showFatalError, recoverAccount } from "./helpers";
-import { Root } from "native-base";


@marcelomorgado Hmm, are you sure removing this won't break the toasts in the account creation / funding flow? I can test that.

You are right! I've just undone the removal.

Okay, thanks. I'll confirm those toasts are working in this branch anyway before merging after you added it back, since this is the branch that started messing with toasts even when Root was still in App.js

I'm glad I checked. Those aren't working on any other screens now. So we'll have to add the Root change on every screen that uses a showInfo, etc. function if we want the toasts everywhere we used to have them.

I'd be potentially okay with removing them from the account creation and funding flow since there's the on-screen text now too, but we at least need the toasts for when the land purchase is complete to work. Maybe that happens back on the ListLandForSaleScreen though where we already have it now?

I've tested it before put Root again on the App component. I think that is desirable be able to create a toast from every component that we want, I don't have a strong opinion about keeping or removing account creation steps toasts.

I have to check if the toast from account creation flow will be shown on list land screen even without Root on the App component. Was that your question?

Yes, we have few options now and that seems the better one. Any idea of the content that this screen will have?

I'd be potentially okay with removing them from the account creation and funding flow since there's the on-screen text now too, but we at least need the toasts for when the land purchase is complete to work. Maybe that happens back on the ListLandForSaleScreen though where we already have it now?

@marcelomorgado Oh actually, if we merge it as-is, the toasts for finishing buying a piece of land do show up back on the list land screen, so we would have all the toasts we absolutely need if we were to merge this with Root just on the ListLand screen and in App.js, which leads to no errors. Thoughts?

Unfortunately, the toast will stop to work on all screen as before, but it's a good option too since we can merge this with no terrible side effects and work on the "coaching" screen and have the toast back to all screens soon.

I see you gave a thumbs up in response to the last message, so I will go forward with the merging as-is plan for now.

Unfortunately, the toast will stop to work on all screen as before, but it's a good option too since we can merge this with no terrible side effects and work on the "coaching" screen and have the toast back to all screens soon.

Okay, sounds good to me.

pcowgill · 2019-04-02T18:41:49Z

Hey @pcowgill , I've found this comment wix/react-native-navigation#4640 (comment) event it isn't totally clear for me, seems that the Root component will not work from App (we can remove it).

Hmm, but for react-native-navigation as opposed to react-navigation, the navigation is using native code and not JavaScript. We're using JavaScript though for navigation since we're using react-navigation. So I don't think the wix (maker of react-native-navigation) issues are a good reference point for this topic.

pcowgill · 2019-04-02T18:43:36Z

I've quickly tried to add it programmatically inside of AppNavigator but my attempts until now don't work. I think will be better do a comment about it and go ahead. Make sense?

If you agree with me, this PR is ready to merge.

I'm on board with the idea of merging with a temporary working solution and a comment, but I'm not sure if removing Root from App.js will introduce any bugs, given that the issue was a wix one.

marcelomorgado · 2019-04-02T18:47:32Z

Oh, thanks for the clarification, I got confused a little. I've changed my previous comment about the issue on the code.

pcowgill · 2019-04-03T17:19:42Z

@marcelomorgado Can you review and approve the latest changes before we merge this? Thanks!

pcowgill · 2019-04-03T17:20:06Z

(Not much has changed - just to follow our usual process)

Removed duplicate loading screen

82b3ac4

pcowgill requested a review from marcelomorgado April 1, 2019 22:26

pcowgill changed the title ~~Removed duplicate loading screen~~ WIP: Removed duplicate loading screen Apr 2, 2019

pcowgill added 3 commits April 2, 2019 09:25

Remove unused Home component

7ea1b58

Style empty my assets and wording fix

80642fb

Capitalization and call assets land in UI

bee0ad0

pcowgill added 3 commits April 2, 2019 10:13

Debugging native-base issue

815fcf9

Updated LandForSaleList snapshot

7c3962b

Removed use of the word asset in UI

110233b

marcelomorgado added 2 commits April 2, 2019 19:09

Removing console.log

0718361

Merge branch 'develop' into feature/no-loading-screen

b93ec2a

marcelomorgado previously approved these changes Apr 2, 2019

View reviewed changes

pcowgill changed the title ~~WIP: Removed duplicate loading screen~~ Removed duplicate loading screen Apr 2, 2019

pcowgill commented Apr 2, 2019

View reviewed changes

Review request

abf7838

marcelomorgado dismissed their stale review via abf7838 April 2, 2019 18:42

Updating comment

47fb80b

Merge branch 'develop' into feature/no-loading-screen

c6015b7

marcelomorgado approved these changes Apr 3, 2019

View reviewed changes

pcowgill merged commit 2e2e970 into develop Apr 3, 2019

pcowgill deleted the feature/no-loading-screen branch April 3, 2019 17:22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Removed duplicate loading screen #237

Removed duplicate loading screen #237

pcowgill commented Apr 1, 2019 •

edited

pcowgill commented Apr 1, 2019

marcelomorgado commented Apr 2, 2019

pcowgill commented Apr 2, 2019

pcowgill commented Apr 2, 2019

pcowgill commented Apr 2, 2019

pcowgill commented Apr 2, 2019

marcelomorgado commented Apr 2, 2019

marcelomorgado commented Apr 2, 2019

pcowgill Apr 2, 2019

marcelomorgado Apr 2, 2019 •

edited

pcowgill Apr 2, 2019

pcowgill Apr 2, 2019

marcelomorgado Apr 2, 2019

marcelomorgado Apr 3, 2019

pcowgill Apr 3, 2019

marcelomorgado Apr 3, 2019

pcowgill Apr 3, 2019

pcowgill Apr 3, 2019

pcowgill commented Apr 2, 2019 •

edited

pcowgill commented Apr 2, 2019

marcelomorgado commented Apr 2, 2019

pcowgill commented Apr 3, 2019

pcowgill commented Apr 3, 2019

Removed duplicate loading screen #237

Removed duplicate loading screen #237

Conversation

pcowgill commented Apr 1, 2019 • edited

Issue link

Auto-close the issue?

Types of changes

pcowgill commented Apr 1, 2019

marcelomorgado commented Apr 2, 2019

pcowgill commented Apr 2, 2019

pcowgill commented Apr 2, 2019

pcowgill commented Apr 2, 2019

pcowgill commented Apr 2, 2019

marcelomorgado commented Apr 2, 2019

marcelomorgado commented Apr 2, 2019

Choose a reason for hiding this comment

marcelomorgado Apr 2, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pcowgill commented Apr 2, 2019 • edited

pcowgill commented Apr 2, 2019

marcelomorgado commented Apr 2, 2019

pcowgill commented Apr 3, 2019

pcowgill commented Apr 3, 2019

pcowgill commented Apr 1, 2019 •

edited

marcelomorgado Apr 2, 2019 •

edited

pcowgill commented Apr 2, 2019 •

edited