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

Some updated + material 3 icon #215

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

quantumde1
Copy link

Material You icon, Android 5.0 backward compatability, Gradle update, dependencies update

@FlorentRevest
Copy link
Member

The gradle and dependencies update look good to me but I don't understand the rationale behind changing the icon ? We should at least get an opinion from our designer @eLtMosen before doing such a change, and this should be thought through in a different context than a dependencies update.

Also, this PR bundles a bunch of precompiled binary files which shouldn't ship as part of our git repositories, like the app/release/ directory.

Also it would be good to explain why certain changes are done like those to app/src/main/java/org/asteroidos/sync/connectivity/ScreenshotService.java as part of the commit message.

@eLtMosen
Copy link
Member

Indeed, a discussion to change the app icon should be out of scope of this PR due to its big impact.
The orange and unicolor icon versions have been tailored to each display situation. And changing them would require testing all those again.
In a PR with visual changes, it is advised to attach screenshots of the changes to speed up the review process and help more people understand the changes and voice an opinion on them.

@quantumde1
Copy link
Author

The gradle and dependencies update look good to me but I don't understand the rationale behind changing the icon ? We should at least get an opinion from our designer @eLtMosen before doing such a change, and this should be thought through in a different context than a dependencies update.

Also, this PR bundles a bunch of precompiled binary files which shouldn't ship as part of our git repositories, like the app/release/ directory.

Also it would be good to explain why certain changes are done like those to app/src/main/java/org/asteroidos/sync/connectivity/ScreenshotService.java as part of the commit message.

When updated Gradle and changed compatability i changed some code for work on Android 5.0+. Icon is changed because i think many android 12+ version users who is own watches on AsteroidOS want to get Material You UI(at least, many people who is using A12 and higher in environment doesnt like non-dynamic icons). And i think it will be nice to show users your attitude to them as developer, people will be happy!
About binaries - im sorry, forget to remove binaries before push. I will remove them ASAP.

@eLtMosen
Copy link
Member

eLtMosen commented Jun 1, 2024

Icon is changed because i think many android 12+ version users who is own watches on AsteroidOS want to get Material You UI(at least, many people who is using A12 and higher in environment doesnt like non-dynamic icons).

This sounds like an improvement for only a subset of users, with potential unknown regressions for others.

Discussing and improving design is a rather involved matter. While we appreciate the initiative, we need to ensure that the proposed icon changes do not negatively impact the user experience across different systems.

To be thorough, it is important to provide screenshots that compare the icons before and after the change for every different UI screen where the icon appears. This should be done for as many impacted systems as possible.

At a minimum, please show the before/after changes on the system and UI you use. It would be helpful to clarify what bothers you about the old icon and visually demonstrate how the new icon improves the experience in the given situations.

This way, reviewers can technically test and evaluate your changes on their systems to check for regressions. However, it is quite time-consuming for reviewers to do this without initial evidence. If you are serious about changing the icon and want to convince the community, it would be very helpful to conduct this initial work and check for regressions yourself.

That is the reason why @FlorentRevest asked to separate the icon changes from your much-welcome updates and fixes. These updates could quickly be merged, while the icon changes involve a community discussion and more preparation. This discussion is ideally done on our Matrix channel, which you could join if you haven't already.
https://app.element.io/#/room/#Asteroid:matrix.org

@quantumde1
Copy link
Author

Screenshot_20240603-004243_Настройки
Screenshot_20240603-004232_Trebuchet
As you can see, now icon is white-orange while without material you, and system color when with.
Okay, i'll cut commits for different branches, so you can merge gradle updates. I was tested my build on android 6.0, its working too, without any issues.

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.

3 participants