Skip to content
This repository has been archived by the owner on Jun 16, 2022. It is now read-only.

Max account decimals before trim and tooltip for whole value #1947

Closed
wants to merge 1 commit into from
Closed

Max account decimals before trim and tooltip for whole value #1947

wants to merge 1 commit into from

Conversation

jorge-cob
Copy link
Contributor

maxLength

Type

Bug Fix, UI Polish...

Context

Account's balance was overflowing the wrapper where there are lot of decimal values in a small screen

Parts of the app affected / Test plan

before maxLength

Copy link
Contributor

@gre gre left a comment

Choose a reason for hiding this comment

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

neat!

@@ -131,6 +140,11 @@ function FormattedVal(props: Props) {
)}
</T>
)

if (showTooltip) {
return <Tooltip render={() => unit && formatCurrencyUnit(unit, val)}> {content} </Tooltip>
Copy link
Contributor

Choose a reason for hiding this comment

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

should the unit check be done in the if? because as far as i read it, if there is no unit, we would have no tooltip right?

if (showTooltip && unit) {
    return <Tooltip render={() => formatCurrencyUnit(unit, val)}> {content} </Tooltip>
}

I imagine this case never present, but code is easier to understand

@gre gre added the internal discussion we are discussing this new feature internally. HODL until there is a decision. label Apr 23, 2019
@gre gre removed the internal discussion we are discussing this new feature internally. HODL until there is a decision. label Apr 26, 2019
@gre
Copy link
Contributor

gre commented Apr 26, 2019

good

Copy link
Contributor

@Arnaud97234 Arnaud97234 left a comment

Choose a reason for hiding this comment

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

Bug found
Account balance should be responsive. By resizing the window, account pane become large enough to display all the balance digits.

Screenshot 2019-04-29 at 12 21 32

How to reproduce

  • Import app.json file (enclosed)
  • On portfolio screen, resize app window.

app.json.txt

@gre
Copy link
Contributor

gre commented Apr 29, 2019

Ok so base on this feedback and fact the window can resize & need to be somewhat responsive, i'm going to suggest a different implementation:

  • For the actual ... rendering, I would prefer we use classical CSS ellipsis https://developer.mozilla.org/en-US/docs/Web/CSS/text-overflow . That's the only true way we can make it correct, because (1) the font is not monospace (2) it have different sizes on different OSs (3) you can resize the window so the width possibilities is infinite.
  • For dynamic presence of the Tooltip, this is getty a bit harder to implement it right I feel. We would need a way to know if the CSS ellipsis is on or not, and you probably need to check for that at each window resize or even re-rendering.. we will have to be very careful about performance of this / tradeoff can be found. I would be ok having a first implementation that always show the tooltip as a first step (even tho i think it's better to not show it always)

@gre
Copy link
Contributor

gre commented Jun 4, 2019

I think it was fixed in Accounts rework

@gre gre closed this Jun 4, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants