-
Notifications
You must be signed in to change notification settings - Fork 27
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
Improved UX and UI #3
Conversation
@slowscript what do you think about my changes? |
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.
Thanks for the PR. I like the changes overall, it's definitely a major improvement.
Here are a few nitpicks however:
- I think the "transfer" cards are a little too tall. Most of the design is pretty compact and this seems a bit out of place.
- There seems to be an issue with the icons. When I run the application in an emulator, they look fine, but in Android Studio and the PNGs they look irregular for some reason.
- I will also need you to rebase your branch onto my branch. You have instead merged my branch into yours, which means that if I merge this as-is, the commits I made will be duplicated.
- The remaining minor issues are in the comments
<string name="theme_settings_title">Theme</string> | ||
<string name="overwriting_setting_title">Allow overwriting</string> | ||
<!-- Theme settings --> | ||
<string-array name="theme_settings_entries_array"> <!-- do not translate this --> |
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 think this should be in a separate xml file (like arrays.xml)
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 did it this way initially. But it's only one array and it's a string one.
I have no idea what causes that. I tried fixing it. I imported them with Android Studio.
My bad. Do you know if there is any way to fix this? Or I need to revert to a backup copy I made before doing this?
I will try even more models, but in my opinion, it is not such a big change to annoy you visually |
Did you import them as SVGs? Android Studio acts weirdly with some SVGs. If that's the case, just export them from the software you made them in as a large enough PNG and import that. That's what I did with my icons (even though better solutions likely exist). If you have a backup right before the merge, it would be probably be best to revert to that and do the rebase. If not, then what I (not a git expert by any means) would do is that 1) I would turn the commits into patches, 2) start with a fresh clone and 3) reapply and commit them one by one. This would also allow you to fix some of the things without making additional commits. Anyway, Google is your friend in this case. I could also try to fix it for you if you want to (but I can't guarantee it will work). |
No. I imported them from Android Studio with Android Studio. They are like the official icon package for Android apps. (they are under the Apache License 2.0 so we can use them). I am gonna fix it myself. Cuz I wanna fix some things. |
Added feedback for the user when needed
I improved some icons I changed most part of strings to resources for more translatability Improved colors
Now you can change the theme in the setting
The icons change colors depending on the theme
@slowscript I fixed it. I fixed the tint problem, the rebase problem and the typo. If you have any suggestions let me know. |
Awesome! About the other things: I've played around with the icons and it seems that no matter what I try, Android Studio still renders them weirdly. I guess this doesn't really matter. What I did find out however is that the PNGs are redundant. Just import the icons as a Vector Asset (instead of an Image Asset). This way only a single XML and no broken PNGs are generated. About the design: I still think that the "transfer" cards should be a little less tall - I'd say something around 80dp and corner radius of 10dp would be ideal. This is a non-issue though. I will probably tweak this along with some other things later. The only thing stopping the merge now are the images. I don't want to have a bunch of useless broken PNGs in the repository. |
@slowscript I removed the pngs in the force push. And now the app uses the xml. About the transfer card, I will tweak them. I'm gonna continue working on this next week. I think I forgot about some icons that need a tint and I have some drawing boards with even more modifications (I am not a UI designer, front-end developer or anything like that btw.). And even more translations. Edit: I figured why the icons looked weird. Android Studio uses some experimental renderer by default. You can disable it in |
Looks good to me now |
I improved the aspect and feel.
Changes I made: