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

Tweak posting row item widths to make currency a bit wider #62

Merged
merged 3 commits into from
Dec 17, 2023

Conversation

Ajnasz
Copy link
Contributor

@Ajnasz Ajnasz commented Dec 13, 2023

On a Nothing Phone 1, on the Add transaction screen the currency were not
visible because the left and right paddings added too much empty space to the
text field and it just scrolled out from the view.

Removed the the padding from the currency field as the it is already aligend to
the center. The padding is not needed, the spacing will be evenly distributed
before and after the text.

Uploaded the fixed version to my phone, opened the "Add transaction" action. My
predefined currency were visible and no scrolling were needed anymore.

On a Nothing Phone 1, on the Add transaction screen the currency were
not visible because the left and right paddings added too much empty
space to the text field and it just scrolled out from the view.

Removed the the padding from the currency field as the currency is
already aligend to the center. The padding is not needed, the spacing
will be evenly distributed before and after the text.
@chvp chvp self-requested a review December 14, 2023 07:00
@chvp
Copy link
Owner

chvp commented Dec 14, 2023

Thanks for your pull request! I did a lot of fiddling with the widths and padding to try to make it work on smaller screens, but evidently not enough. Could you post some before/after screenshots? I'll test it out on my device as well (probably on the weekend), but my screen is pretty big...

I fear that fully removing the end padding when the currency field is the last field won't look great. (One thing I tried, but couldn't figure out was removing/reducing the inner padding of the text fields, because that's actually where a lot of horizontal space is lost.)

@Ajnasz Ajnasz force-pushed the currency-field-display branch from 61e0938 to f54f0cf Compare December 15, 2023 09:19
@Ajnasz
Copy link
Contributor Author

Ajnasz commented Dec 15, 2023

Hi!

My first assumption about the issue was wrong, as the problem is not the padding, instead the fact that I was using a 3 char long currency, which couldn't fit into the field. You can see the issue in the emulator too if you set a longer currency in the settings.

I added another change to fix that. It looks like the weight was misused. From the documentation I've found the values are ratios and not percentage of the available space. According to that I changed the weights, so the currency field will be a bit wider and the 3 character currency will fit too.

Here are screenshots from my phone:

wrong1:

wrong2:

good currency before amount:

good currency after amount:

The currency was too narrow to fit a 3 char currency there and the text
was scrolled out. To fix, increase the size of the currency field.
Also change how text weights defined:
Based on https://developer.android.com/jetpack/compose/modifiers#weight-in-row-and-column
it's not a percentage based calculation, but more like a ratio.
@Ajnasz Ajnasz force-pushed the currency-field-display branch from f54f0cf to f162bb9 Compare December 15, 2023 09:33
Copy link
Owner

@chvp chvp left a comment

Choose a reason for hiding this comment

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

(Percentages are equivalent to weights, so I wouldn't say the previous usage was wrong.)

@chvp chvp changed the title fix currency field display Tweak transaction row item widths to make currency a bit wider Dec 17, 2023
@chvp chvp changed the title Tweak transaction row item widths to make currency a bit wider Tweak posting row item widths to make currency a bit wider Dec 17, 2023
@chvp chvp merged commit 677dc92 into chvp:main Dec 17, 2023
3 checks passed
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.

2 participants