-
-
Notifications
You must be signed in to change notification settings - Fork 18
Improve UI in RTL mode & Improve performance of DropdownMenu #75
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
Conversation
|
I think we should move the calculator history to a separate screen to save some space on the screen. |
.idea/compiler.xml
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please remove the commited changes to .idea.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oops.
I think it should be added to .gitignore.
My IDE automatically edited these files without me noticing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I agree.
| val itemHeight by remember { | ||
| mutableStateOf(45.dp) | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
itemHeight doesn't need to be a mutable state since it's never modified, you can place use 45.dp directly in dropDownHeight.
app/src/main/java/net/youapps/calcyou/ui/screens/ConverterScreen.kt
Outdated
Show resolved
Hide resolved
| } | ||
| LazyColumn( | ||
| modifier = Modifier | ||
| .width(width) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| .width(width) | |
| .fillMaxWidth() |
didn't work here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried .fillMaxWidth() and it crashes whenever you open the dropdown menu
Bnyro
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the PR!
I've also noticed the performance issues when opening the currency converter dropdown, so these changes are very welcome :)
I'm not sure about that, I find it quite useful to have the history on the same screen. Sometimes I need some result of a previous calculation for the next expression I want to calculate, so it makes sense to see it at the same glance.
True, we should fix that. |
Fixed with ddbb5ce. |
…en.kt Co-authored-by: Bnyro <bnyro@tutanota.com>
I also changed some variable names to make more sense
It is hard to find the result of previous calculation because you only see 2 rows in the history box and you have to keep scrolling until you find it. |
Adding search in the currency screen may solve the issue? |
At least from my experience it's simple enough because I don't do more than 10 calculations in a session, I guess we won't find an agreement here :)
I agree on that, pull requests welcome. |
Bnyro
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you!
I fixed UI in RTL Mode.
I think the calculator layout should stay as it is in RTL mode.
Those parenthesis were annoying.
I also improved the performance of the Dropdown menu of the UnitConverter screen.
The difference is significant in currencies screen as it has the largest dropdown menu.
It now opens much faster since I'm using LazyColumn.