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

[DDW-81] Improve redirecting logic of network store #723

Merged

Conversation

DominikGuzei
Copy link
Member

This PR greatly simplifies the network status store and improves the booting UX:

  • Removes any redirecting / URL / screen handling logic from NetworkStatusStore
  • Introduces new Root route + component which handles the conditional logic of what to display based on various states
  • Show the loading spinner while waiting for the time difference request to finish (instead of showing 100% syncing message for 5 seconds)
  • The original URL is now preserved (because we are not doing any redirects), this means that you can now reload the app on sub-screens like "wallet / send" and it will return to the same 😉

All tests pass as before, also tested everything with API=etc and there are not lint or flow issues.

@DominikGuzei
Copy link
Member Author

@nikolaglumac this is now also ready to be reviewed!

@nikolaglumac nikolaglumac changed the title [DDW-81] simplify app booting logic Chore/ddw 81 Improve redirecting logic of network store Mar 3, 2018
@@ -32,7 +31,7 @@ export default class LoadingPage extends Component<InjectedProps> {
allowedTimeDifference={ALLOWED_TIME_DIFFERENCE}
isConnecting={isConnecting}
syncPercentage={syncPercentage}
isLoadingDataForNextScreen={isLoadingWallets}
isLoadingDataForNextScreen={!isSyncing || isSynced}
Copy link
Contributor

Choose a reason for hiding this comment

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

@DominikGuzei I believe we should take this opportunity to refactor isSyncing and isSynced logic in the NetworkStatusStore: https://github.com/input-output-hk/daedalus/blob/development-webpack-fork/source/renderer/app/stores/NetworkStatusStore.js#L140-L151

}

setup() {
async setup() {
Copy link
Contributor

Choose a reason for hiding this comment

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

@DominikGuzei can you explain what is the reason for async here?

this._updateSyncProgress, SYNC_PROGRESS_INTERVAL
);
if (environment.isAdaApi()) {
this._updateLocalTimeDifferencePollInterval = setInterval(
Copy link
Contributor

Choose a reason for hiding this comment

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

@DominikGuzei I fear this update is in collision with the following improvement: #719

Copy link
Member Author

@DominikGuzei DominikGuzei May 25, 2018

Choose a reason for hiding this comment

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

No it isn't the code is even better now, you have to read through it carefully 😉

Logger.debug('Stopped polling local time difference');
if (this._timeDifferencePollInterval) clearInterval(this._timeDifferencePollInterval);
_updateLocalTimeDifferenceWhenConnected = async () => {
if (this.isConnected) await this._updateLocalTimeDifference();
Copy link
Contributor

Choose a reason for hiding this comment

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

@DominikGuzei this call should be made only in case of environment.isAdaApi()

Copy link
Contributor

@nikolaglumac nikolaglumac left a comment

Choose a reason for hiding this comment

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

@DominikGuzei we should discuss how to proceed with this PR on Monday. This part of the application is very sensitive (as you already know) and I think we should re-define how to handle all the cases.

@nikolaglumac nikolaglumac changed the base branch from development-webpack-fork to develop March 27, 2018 22:10
@nikolaglumac
Copy link
Contributor

@DominikGuzei can you please merge in latest develop in your branch? I will review this PR after you do that. Thanks!

@nikolaglumac
Copy link
Contributor

@DominikGuzei please merge in latest develop so that this one can finally be reviewed :) Thanks!

@DominikGuzei DominikGuzei removed the WIP label May 25, 2018
@DominikGuzei
Copy link
Member Author

@nikolaglumac i fixed the conflicts with latest develop branch and all tests pass! Please review carefully as these are nice improvements.

@nikolaglumac nikolaglumac changed the title Chore/ddw 81 Improve redirecting logic of network store [DDW-81] Improve redirecting logic of network store May 27, 2018
@DominikGuzei DominikGuzei removed the WIP label Jun 2, 2018
@nikolaglumac
Copy link
Contributor

Let's wait with merging this PR until the 1.3 release cut off date.

@DominikGuzei
Copy link
Member Author

@nikolaglumac this is ready for review (again)

@DominikGuzei DominikGuzei removed the WIP label Jun 21, 2018
@nikolaglumac
Copy link
Contributor

@DominikGuzei reviewing it now! (again)

@nikolaglumac nikolaglumac dismissed their stale review June 21, 2018 14:44

Issues fixed

Copy link
Contributor

@nikolaglumac nikolaglumac left a comment

Choose a reason for hiding this comment

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

Great work @DominikGuzei!
The only issue is that if you have no wallets and you try top open Ada redemption screen from the application menu you end up on the Add wallet screen :(

@@ -62,6 +62,7 @@ export default class AdaRedemptionStore extends Store {
ipcRenderer.on(PARSE_REDEMPTION_CODE.ERROR, this._onParseError);
this.registerReactions([
this._resetRedemptionFormValuesOnAdaRedemptionPageLoad,
this._redirectToAddWalletBeforeRedemption,
Copy link
Contributor

Choose a reason for hiding this comment

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

@DominikGuzei I don't think we need this redirect anymore as handle this case by showing special content on the Ada redemption screen.

@@ -299,4 +300,12 @@ export default class AdaRedemptionStore extends Store {
this.decryptionKey = null;
};

_redirectToAddWalletBeforeRedemption = () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

@DominikGuzei this is not needed anymore!

@DominikGuzei DominikGuzei removed the WIP label Jun 22, 2018
@DominikGuzei
Copy link
Member Author

@nikolaglumac the issues with ada redemption page are fixed now!

Copy link
Contributor

@nikolaglumac nikolaglumac left a comment

Choose a reason for hiding this comment

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

Looks and works great 🎉
Approved 👍

@nikolaglumac
Copy link
Contributor

@DominikGuzei awesome work! This PR is ready to be merged as soon as CI is done ;)

@nikolaglumac nikolaglumac merged commit dc58c73 into develop Jun 22, 2018
@nikolaglumac nikolaglumac deleted the chore/ddw-81-improve-url-handling-in-network-store branch June 22, 2018 11:30
This was referenced Oct 16, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants