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

Responsive Settings and Transaction Details #2215

Merged
merged 5 commits into from
Nov 1, 2019

Conversation

bgptr
Copy link
Collaborator

@bgptr bgptr commented Oct 15, 2019

This closes #1951, closes #2000, and closes #2201

Copy link
Member

@vctt94 vctt94 left a comment

Choose a reason for hiding this comment

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

Thanks for this PR.

In the next ones, try to keep one issue per PR (unless they are very related, like #2000 and #2201) because it is easier to review.

The settings part LGTM.

On tx detail page, I notice the to address and tx fee is missing when tx is pending

image

Also the tx address is missing in the confirmed ones

@bgptr
Copy link
Collaborator Author

bgptr commented Oct 16, 2019

Thank you for the review.

Sorry for the multi-issue PR, I won't do that again.

I think on the screenshot there is no transaction fee because it's an incoming transaction.
The to address row is missing, but it's missing in the master branch too.
Am I right or just looking for it in the wrong place?

Meanwhile, I find an issue with the header. It has a fixed height and it did not increase properly. Now it is corrected.

Copy link
Member

@vctt94 vctt94 left a comment

Choose a reason for hiding this comment

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

Good job on the fixed header height bug.

You are right about the pending transaction, but we have the output addresses at tx. Can you add them to the headers?

@vctt94
Copy link
Member

vctt94 commented Oct 18, 2019

Also #2222 made me realize that the save button is not showing at the screen in smaller width. Can you fix that, as well?

@bgptr
Copy link
Collaborator Author

bgptr commented Oct 18, 2019

I am on the output addresses.

I think this PR already solve #2222. At least I can not reproduce it.

@vctt94
Copy link
Member

vctt94 commented Oct 18, 2019

Weird, here it doesn't show, look:

image

@bgptr
Copy link
Collaborator Author

bgptr commented Oct 18, 2019

Maybe I messed up my git repo. Now I synced the PR.
The header should be in the scrollable area.

image

@vctt94
Copy link
Member

vctt94 commented Oct 18, 2019

Yeah, I noticed. You brought some master's commits to your PR. Can you rebase it?

@bgptr bgptr force-pushed the responsive_settings_and_txdetails branch from 72748fb to 22a1eb4 Compare October 18, 2019 14:20
@bgptr
Copy link
Collaborator Author

bgptr commented Oct 18, 2019

I've rebased it and added the output addresses.

@vctt94
Copy link
Member

vctt94 commented Oct 20, 2019

Are you sure? Here it still does not show

image

@bgptr
Copy link
Collaborator Author

bgptr commented Oct 21, 2019

Please try it now.

@vctt94
Copy link
Member

vctt94 commented Oct 21, 2019

Thanks, it looks fine now. I have just one more comment. Can you edit this PR so the issues close when merging it?

When writing in topics like that, github doesn't close the issues when merging the PR.
To properly close you need to specify issue by issue.

From github documentation: For example, This closes #34, closes #23, and closes example_user/example_repo#42 would close issues #34 and #23 in the same repository, and issue #42 in the "example_user/example_repo" repository.

source: https://help.github.com/en/github/managing-your-work-on-github/closing-issues-using-keywords#closing-multiple-issues

@bgptr
Copy link
Collaborator Author

bgptr commented Oct 21, 2019

Sure. I edited it.

Copy link
Member

@vctt94 vctt94 left a comment

Choose a reason for hiding this comment

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

Thanks!

@alexlyp
Copy link
Member

alexlyp commented Oct 30, 2019

Please resolve these conflicts and then it should be good to go.

@bgptr bgptr force-pushed the responsive_settings_and_txdetails branch from 6fc9815 to 722cc5d Compare October 30, 2019 21:31
@alexlyp alexlyp merged commit 81a9c2c into decred:master Nov 1, 2019
@bgptr bgptr deleted the responsive_settings_and_txdetails branch February 9, 2021 18:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants