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

make send asset a modal #72

Closed
wants to merge 1 commit into from
Closed

Conversation

matjazc
Copy link

@matjazc matjazc commented Nov 16, 2020

No description provided.

@KarishmaBothara
Copy link

Have tested it functionally on my local.. works well.. Can we remove the 'Send assets' from the left panel.. as per design now that will part of accounts page..

@aliXsed
Copy link

aliXsed commented Nov 17, 2020

The code looks good to me as a non React/js expert. The PR can be improved by:

Also, the branch that you worked on could be better named something like feature/31-modal-send-asset. The first part "feature" specifies this is a new feature. If you were fixing an issue it could start with fix/ and so on. 31 is an example of an issue number that could be used in the branch name.
Having said this, you may notice we haven't fully followed the above-mentioned standard in our previous PRs and branch names. At the moment we take the above practice as an improvement guideline and we'd try to get better at it.

@matjazc
Copy link
Author

matjazc commented Nov 17, 2020

Have tested it functionally on my local.. works well.. Can we remove the 'Send assets' from the left panel.. as per design now that will part of accounts page..

Sure, I will remove side bar item

@matjazc
Copy link
Author

matjazc commented Nov 17, 2020

The code looks good to me as a non React/js expert. The PR can be improved by:

Also, the branch that you worked on could be better named something like feature/31-modal-send-asset. The first part "feature" specifies this is a new feature. If you were fixing an issue it could start with fix/ and so on. 31 is an example of an issue number that could be used in the branch name.
Having said this, you may notice we haven't fully followed the above-mentioned standard in our previous PRs and branch names. At the moment we take the above practice as an improvement guideline and we'd try to get better at it.

This is a nice set of good practices, I agree with all points. Thanks for raising this.

@jordy25519
Copy link

is there a way to close modal?

@matjazc matjazc closed this Nov 19, 2020
@matjazc
Copy link
Author

matjazc commented Nov 19, 2020

new PR created

@matjazc matjazc deleted the make-send-asset-a-modal branch November 19, 2020 01:09
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.

4 participants