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

#2944. promisify send modal #2976

Closed

Conversation

OpenLedgerApp
Copy link

@OpenLedgerApp OpenLedgerApp commented Jul 24, 2019

General

Closes #2944

refactoring

General

Please make sure the following is done:

Code Preparation

Please review all your changes one last time before committing

  • Check for unused code
  • No unrelated changes are included
  • None of the changed files are reformatting only
  • Code is self explanatory or documented
  • All written text is properly translated (english language)

Testing

The branch has been tested on the following browsers (desktop and mobile view)

  • Chrome
  • Opera
  • Firefox
  • Safari

User interface changes

Delete this section if there weren't any UI changes. Please make sure you tested your changes in all themes

  • Dark
  • Light
  • Midnight

Please provide screenshots/licecap of your changes below

@startailcoon
Copy link
Contributor

@OpenLedgerApp what was your time on this?

@OpenLedgerApp
Copy link
Author

@startailcoon I suppose about 3h

@startailcoon
Copy link
Contributor

startailcoon commented Jul 29, 2019

Feels like a lot for such a small change, but I assume it's been fully debugged and tested.

I would like a more detailed explanation of the time taken.

@@ -168,19 +168,47 @@ class SendModal extends React.Component {
});
}

fetchToAccount(toName) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why did you make both fetchTo and fetchFrom, and then even with an accountName argument?

Copy link
Author

Choose a reason for hiding this comment

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

changed

@sschiessl-bcp
Copy link
Contributor

sschiessl-bcp commented Jul 29, 2019

@startailcoon I suppose about 3h

What does suppose mean? I would expect proper time tracking from company contributions. There are also still several instances of un-promisified data fetching.

@startailcoon is providing a fix for the account selector, so I please do not continue at the moment

@OpenLedgerApp
Copy link
Author

@startailcoon
1h - debugging
1h 30m - implementation
30m - testing

@sschiessl-bcp
Copy link
Contributor

Needs re-evaluation with restructuring of SendModal due to FeeAssetSelector

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.

[3] Promisify SendModal
3 participants