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

Mario/2670 add max amount #2956

Merged
merged 46 commits into from
Sep 6, 2019
Merged

Mario/2670 add max amount #2956

merged 46 commits into from
Sep 6, 2019

Conversation

mariopino
Copy link
Contributor

Closes #2670

Description:

Add max button to amount field in transfer, delegate and undelegate modals.

Thank you! 🚀


For contributor:

  • Added changes entries. Run yarn changelog for a guided process.
  • Reviewed Files changed in the github PR explorer
  • Attach screenshots of the UI components on the PR description (if applicable)
  • Scope of work approved for big PRs

For reviewer:

  • Manually tested the changes on the UI

@faboweb
Copy link
Collaborator

faboweb commented Sep 5, 2019

So this is missing the warning if there is no more funds for fees left as discussed in the issue

@jbibla
Copy link
Collaborator

jbibla commented Sep 5, 2019

i might propose we add this button to TmField with a prop maxAmount indicating whether or not it should be shown and the amount.

this would mean we wouldn't have to duplicate the HTML.

do you think this would be a good approach?

@mariopino
Copy link
Contributor Author

Added message:

Warning! you should probably leave some left over for fees

@codecov
Copy link

codecov bot commented Sep 5, 2019

Codecov Report

Merging #2956 into develop will increase coverage by 0.03%.
The diff coverage is 58.33%.

@@             Coverage Diff             @@
##           develop    #2956      +/-   ##
===========================================
+ Coverage    92.83%   92.87%   +0.03%     
===========================================
  Files          129      129              
  Lines         2234     2259      +25     
  Branches       389      388       -1     
===========================================
+ Hits          2074     2098      +24     
- Misses         155      156       +1     
  Partials         5        5
Impacted Files Coverage Δ
src/components/common/TmField.vue 100% <ø> (ø) ⬆️
src/ActionModal/components/SendModal.vue 92.68% <50%> (-7.32%) ⬇️
src/ActionModal/components/UndelegationModal.vue 95.45% <66.66%> (-4.55%) ⬇️
src/ActionModal/components/DelegationModal.vue 97.14% <66.66%> (-2.86%) ⬇️
src/ActionModal/components/ActionModal.vue 33.09% <0%> (ø) ⬆️
src/vuex/modules/transactions.js 100% <0%> (ø) ⬆️
src/components/governance/PageProposals.vue
src/components/governance/TabProposals.vue 100% <0%> (ø)
src/vuex/getters.js 82.25% <0%> (+1.55%) ⬆️
src/vuex/modules/blocks.js 100% <0%> (+1.69%) ⬆️
... and 1 more

@codecov
Copy link

codecov bot commented Sep 5, 2019

Codecov Report

Merging #2956 into develop will increase coverage by 0.25%.
The diff coverage is 100%.

@@             Coverage Diff             @@
##           develop    #2956      +/-   ##
===========================================
+ Coverage    92.83%   93.09%   +0.25%     
===========================================
  Files          129      129              
  Lines         2234     2258      +24     
  Branches       389      388       -1     
===========================================
+ Hits          2074     2102      +28     
+ Misses         155      151       -4     
  Partials         5        5
Impacted Files Coverage Δ
src/components/common/TmField.vue 100% <ø> (ø) ⬆️
src/ActionModal/components/SendModal.vue 100% <100%> (ø) ⬆️
src/ActionModal/components/UndelegationModal.vue 100% <100%> (ø) ⬆️
src/ActionModal/components/DelegationModal.vue 100% <100%> (ø) ⬆️
src/ActionModal/components/ActionModal.vue 33.09% <0%> (ø) ⬆️
src/vuex/modules/transactions.js 100% <0%> (ø) ⬆️
src/components/governance/PageProposals.vue
src/components/governance/TabProposals.vue 100% <0%> (ø)
src/vuex/getters.js 82.25% <0%> (+1.55%) ⬆️
src/vuex/modules/blocks.js 100% <0%> (+1.69%) ⬆️
... and 1 more

@colw
Copy link
Contributor

colw commented Sep 6, 2019

i might propose we add this button to TmField with a prop maxAmount indicating whether or not it should be shown and the amount.

this would mean we wouldn't have to duplicate the HTML.

do you think this would be a good approach?

Good suggestion.

Looks great, I've no other comments apart from that.

@faboweb
Copy link
Collaborator

faboweb commented Sep 6, 2019

i might propose we add this button to TmField with a prop maxAmount indicating whether or not it should be shown and the amount.

TmField is already super cluttered. If we want to do this, let's create another component AmountInput or similar which has limited functionality. We thought about this here #1885. I propose we don't do the refactor here and I reopen the according issue.

@faboweb faboweb merged commit ef1a9a9 into develop Sep 6, 2019
@faboweb faboweb deleted the mario/2670-add-max-amount branch September 6, 2019 12:20
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.

add "max" button to amount fields
4 participants