Skip to content
This repository has been archived by the owner on Mar 23, 2023. It is now read-only.

fix: add truncate filter to the wallet balance in the profile #1634

Merged
merged 5 commits into from
Feb 14, 2020
Merged

fix: add truncate filter to the wallet balance in the profile #1634

merged 5 commits into from
Feb 14, 2020

Conversation

brenopolanski
Copy link
Contributor

@brenopolanski brenopolanski commented Jan 29, 2020

Summary

Fixes #1630.

Checklist

  • Documentation (if necessary)
  • Tests (if necessary)
  • Ready to be merged

@ghost ghost added Complexity: Low Less than 64 lines changed. Type: Bugfix The pull request fixes an incorrect functionality or behaviour. labels Jan 29, 2020
@@ -38,7 +38,7 @@
</div>

<span class="font-bold my-2 text-lg">
{{ profileBalance(profile) }}
{{ profileBalance(profile) | truncate(15) }}
Copy link
Member

Choose a reason for hiding this comment

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

Could you add tests for this, please. An alternative place to truncate could be in the profileBalance method as that already has 1 test, just add additional tests for longer variations.

Copy link
Contributor

@dated dated Jan 31, 2020

Choose a reason for hiding this comment

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

Wouldn't the truncate css class a better alternative here? Truncating like above will yield unexpected results for locales where the currency symbol follows the amount, i.e. the symbol will potentially be removed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@alexbarnsley tests added.

Copy link
Member

Choose a reason for hiding this comment

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

@brenopolanski I actually like @dated's suggestion to use the truncate css class 😅 Would you mind trying that approach instead please. Sorry, I only just saw dated's reply

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@alexbarnsley @dated i tested using truncate css class and the result is better:

Screenshot from 2020-02-12 11-43-32

For the truncate class works with <span> tag it's necessary to use the display: inline-block property, then the .my-2 class works well now.

@alexbarnsley alexbarnsley added the Status: Needs Testcase The issue or pull request relates to a feature that needs test coverage. label Jan 31, 2020
@ghost
Copy link

ghost commented Jan 31, 2020

Your pull request doesn't have a test case, which is a requirement for it to be merged. Please provide it and one of the developers will review it before merging.

@brenopolanski
Copy link
Contributor Author

brenopolanski commented Feb 12, 2020

@alexbarnsley can you review this PR again?

@alexbarnsley alexbarnsley merged commit 45a65fa into ArkEcosystem:develop Feb 14, 2020
@brenopolanski brenopolanski deleted the fix/profile-wallet-balance branch February 14, 2020 18:21
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Complexity: Low Less than 64 lines changed. Status: Needs Testcase The issue or pull request relates to a feature that needs test coverage. Type: Bugfix The pull request fixes an incorrect functionality or behaviour.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

The wallet balance leaves the grid div margins
3 participants