Minor tweaks of button alignment and queue spacing #233
Conversation
|
When the repeat mode is activated in the queue, it doesn't show that it is activated. |
|
@mostafaalagamy Thank you for testing! This should be resolved now. I hope ... |
It's resolved, but I think there's a conflict with the PR (unify multiselect look with playlist screen). Please review it. |
|
That's possible. When I cherry picked the other commit to this branch there were no problems but I don't know what it'll look like when merging. But is there a way to resolve conflicts right now anyway? I think I have to wait until one of the PRs is (potentionally) merged and then I can fix the other one. But frankly, I don't know much about git ... |
I think after the merger it will be fix , thanks. |
| top = ListItemHeight, | ||
| bottom = ListItemHeight, | ||
| top = ListItemHeight + 12.dp + 8.dp, | ||
| bottom = ListItemHeight + 8.dp, |
There was a problem hiding this comment.
this is rather bad practice to "hardcode" values like this
There was a problem hiding this comment.
Yes, you're completely right, but I wasn't sure how it was handled in this app. In my own I always use dimension resources. But regarding the Queue maybe we could use Scaffold and put the upper and lower bars into the corresponding "topBar" and "bottomBar" letting the Scaffold measure their heights, so theres no need for a custom layout?
There was a problem hiding this comment.
But regarding the 8.dp spacing ... I mean its hardcoded all over the app:
I personally favor dimension resources, but it doesn't seem to be used here and what would be the alternative?
|
Thanks a lot for this PR, sorry for the time it took for me to accept it, I had a lot of things going on!
I agree with you, it's how it should be handled, but right now I would gladly focus on fixing bugs/new features, but one day, that will be done ! |
To be honest, those things really bugged me, maybe I'm just weird D:
I'd appreciate some feedback, especially if I broke some conventions. Both the
IconButtoninPlayerand theResizableIconButtoninQueuewere the only ones of their kind in their file, so I figured it would be more consistent to replace them with their counterpart.