Dynamically resize Home's balance label. #890
Conversation
There was a problem hiding this comment.
@valentinewallace I rebased this on the current master and added feedback
| width: 250, | ||
| }, | ||
| }) | ||
| ); |
There was a problem hiding this comment.
Why does the wrapper need a width?
There was a problem hiding this comment.
Just tested removing the diff on home.js. It didn't change anything.
There was a problem hiding this comment.
I get this UI bug when I don't set the width in the wrapper:
There was a problem hiding this comment.
I think it's because adjustFontSizeToFit needs a reference point to know when resizing is necessary?
There was a problem hiding this comment.
@valentinewallace I think the issue with the btc unit behind the balance is causing adjustFontSizeToFit to fail. If you display then number as fiat it scales as intended.
There was a problem hiding this comment.
Ah okay, so are you saying it's ok to leave the width as is?
valentinewallace
force-pushed
the
resize-balance
branch
from
3b0b1ae to
811bb09
Compare
| width: 250, | ||
| }, | ||
| }) | ||
| ); |
There was a problem hiding this comment.
Just tested removing the diff on home.js. It didn't change anything.
src/component/label.js
Outdated
| lineHeight: font.lineHeightXXXL, | ||
| lineHeight: null, | ||
| marginLeft: 10, | ||
| marginRight: 10, |
There was a problem hiding this comment.
The home screen already has 20 padding on the left and right. Margin shouldn't be required.
src/component/label.js
Outdated
| }, | ||
| unit: { | ||
| fontFamily: 'WorkSans Regular', | ||
| fontSize: font.sizeL, | ||
| lineHeight: 60, | ||
| marginLeft: 10, |
There was a problem hiding this comment.
This marginLeft: 10 is still required if marginRight on the numeral is removed.
valentinewallace
force-pushed
the
resize-balance
branch
from
811bb09 to
7191f1c
Compare
src/view/home.js
Outdated
| this.setState({ height: event.nativeEvent.layout.height }); | ||
| }} | ||
| style={{ paddingTop: calculateTopPadding(this.state.height) }} | ||
| > |
There was a problem hiding this comment.
Views should not contain any logic. Is there a reason we need to setState here?
There was a problem hiding this comment.
We need the height of the balance to dynamically calculate the paddingTop of the btc unit.
I'll try switching from setState to a normal action instead!
src/helper.js
Outdated
| return 10; | ||
| } | ||
| return 5; | ||
| }; |
There was a problem hiding this comment.
Why is a component specific function in the generic helper module? We should probably move this to the corresponding component.
There was a problem hiding this comment.
let me know if you like my solution!
This is to ensure it stays level with the balance itself.
…e unit in theHome screen balance.
valentinewallace
force-pushed
the
resize-balance
branch
from
7191f1c to
647dccf
Compare
|
Closing in favor of #1295 |
Closes #879.