-
-
Notifications
You must be signed in to change notification settings - Fork 30
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
UI Improvements #104
UI Improvements #104
Conversation
- Added corner radius to timer card - Added padding to clock card - Reduced spacer between main clock and time zone cards to the same value as padding - Set padding to the same values in all tabs
@@ -107,12 +111,13 @@ fun Button( | |||
|
|||
onOperation(NumberKeypadOperation.AddNumber(number)) | |||
}, | |||
modifier = Modifier.size(buttonSize) | |||
modifier = Modifier.size(buttonSize), | |||
color = buttonColor |
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.
color = buttonColor | |
color = MaterialTheme.colorScheme.surfaceColorAtElevation(1.dp) |
Button(number = "3", buttonSize, onOperation) | ||
Button(number = "1", buttonSize, buttonColor, onOperation) | ||
Button(number = "2", buttonSize, buttonColor, onOperation) | ||
Button(number = "3", buttonSize, buttonColor, onOperation) |
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 buttonColor arguments everywhere here.
And you can remove that merge commit by running |
Add material colors to keypad the right way
I made the suggested changes. Makes a lot more sense to do it that way :) |
I was scared that this might override my local changes. I'm new to coding in general and thought that I could get away with focusing on learning Kotlin and how to use AndroidStudio and jetpackCompose, which is already quite a lot to take in. But I see now, that I will also have to learn how to use git properly, because if I don't know how this all works, I will probably make more mistakes. |
You can test this without breaking anything by checking out to a new branch git fetch upstream git rebase upstream/main` |
Thank you for taking the time to give me some useful tips. I highly appreciate it! :) |
I'll squash merge these changes to keep our commit log clean, because I'm too lazy for debasing your branch currently 😆 |
Please make sure to reset your branch/fork to be up to date with my repo before opening a new PR (can be done via the GitHub Ui) |
Thank you. I have watched some videos about Git. I think the problem was, that my fork was setup as origin and upstream. Sorry for the messy PR. I will set this up correctly now so future PR won't look like this. |
I noticed some more things about the ui that could be improved. The number keypad in the timer tab doesn't use the material you color scheme. The timer and stopwatch icons in the navbar are swapped. The stopwatch should be the clock with the buttons on top. Also all the icons in the navbar are outlined except the stopwatch icon. The stopwatch circle could be a little larger since it's the only thing on screen in the stopwatch tab. Here are is a preview of the changes I made:
When trying to fix the stopwatch icon using .filled, I noticed that all the navbar icons use .filled (.default), but only the stopwatch icon is actually drawn with fill and I couldn't figure out why. I fixed it by setting the stopwatch icon to .outlined, but there might be another way to do this that I don't know off.
Also when pushing the changes to my fork I must have made some newbie mistake, because there was another commit created with some translation updates. I don't know how to revert a this commit and cherry-picking the one I want to a new branch didn't work. But as far as I can tell, all the changes in that commit are already present in the main, so this shouldn't be a problem. If it is, please tell me how I can fix this. Hope you like the changes :)