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

Material Design 3 migration #872

Closed
wants to merge 7 commits into from
Closed

Material Design 3 migration #872

wants to merge 7 commits into from

Conversation

Bnyro
Copy link
Contributor

@Bnyro Bnyro commented Jun 12, 2023

Hey,

I recently saw this project has been revived and have been really happy to see the development continue.
Since the app's current design is already some years behind the current trend, I thought it'd be awesome to start with a new, improved and more modern UI.

Therefore this PR attempts to move the app to Material Design 3. Please note that I haven't done any Unit or UI tests and hence can't tell if some of them are broken now (in theory there shouldn't be anything in these changes causing it!).

Please note that the changes required me to update some dependencies (AGP to 7.4.2, Material library to 1.7.0, Gradle to 7.5, appcompat to 1.5.1) and the SDK to 32. I hope there aren't any new issues coming up with that, at least from my testing it seems fine.

There are still two issues I'm fighting with and can't really figure out how to fix because I'm not that familiar with the app yet:

  1. When changing the network in the navigation drawer, the UI freezes and the app crashes after some seconds. However, after restarting the app, the wanted network got selected. That's been related to the dependency updates too, fixed by downgrading some libs.
  2. Now tracked in Bump sdk to 32 and update dependencies for MD3 #873

I hope that changes are welcome (since I forgot to open an issue first) and that someone can test the changes or assist with the two bugs above. Of course I don't claim the changes to be perfect, but in my eyes the app looks pretty neat after applying these.

Cheers

Edit: The screenshots below were made on Android 13, so the app uses dynamic wallpaper based colors. For Android 11 and below, the colors should be similar to the current ones as they're based on the former primary color and generated with https://m3.material.io/theme-builder.

@cla-bot
Copy link

cla-bot bot commented Jun 12, 2023

Thank you for your pull request and welcome to our community! We require contributors to sign our Contributor License Agreement, and we don't seem to have the user @Bnyro on file. In order for your code to get reviewed and merged, please explicitly state that you accept the agreement. Alternatively, you can add a commit that adds yourself to https://github.com/grote/Transportr/blob/master/.clabot

@Bnyro
Copy link
Contributor Author

Bnyro commented Jun 12, 2023

(Yes, I do agree with the CLA)

@Bnyro
Copy link
Contributor Author

Bnyro commented Jun 12, 2023

Screenshot_20230612-193416_Transportr Devel
Screenshot_20230612-193421_Transportr Devel
Screenshot_20230612-193408_Transportr Devel
Screenshot_20230612-193358_Transportr Devel
Screenshot_20230612-193322_Transportr Devel
Screenshot_20230612-193309_Transportr Devel
Screenshot_20230612-193233_Transportr Devel
Screenshot_20230612-193141_Transportr Devel

@pt2121
Copy link
Collaborator

pt2121 commented Jun 13, 2023

Thanks for the contribution. Personally, I'm glad to see the update to material 3.
Ideally, we should have a separate PR to update the compile SDK and target SDK. This would help the review.
Are you willing to create a PR for that first? #869
And, as you've already said, we'd need to fix the issues which you mentioned.

@Bnyro
Copy link
Contributor Author

Bnyro commented Jun 14, 2023

Sure, opened at #873

@Bnyro
Copy link
Contributor Author

Bnyro commented Jun 15, 2023

Update: both bugs were caused by dependency changes, downgrading them again fixed the issues.

@cla-bot
Copy link

cla-bot bot commented Jun 15, 2023

Thank you for your pull request and welcome to our community! We require contributors to sign our Contributor License Agreement, and we don't seem to have the user @Bnyro on file. In order for your code to get reviewed and merged, please explicitly state that you accept the agreement. Alternatively, you can add a commit that adds yourself to https://github.com/grote/Transportr/blob/master/.clabot

@cla-bot cla-bot bot added the cla-signed ✔️ The Contributor Licence Agreement was signed by all contributors. label Jul 24, 2023
@Bnyro
Copy link
Contributor Author

Bnyro commented Jul 24, 2023

Rebased on top of the updated dependencies from my other PR to resolve conflicts.

@pt2121
Copy link
Collaborator

pt2121 commented Jul 30, 2023

There is a compile error in DateUtilsTest.kt. I haven't looked closely but I guess some resources were removed by mistake.

> Task :app:compileDebugUnitTestKotlin FAILED
e: /home/pt/code/forks/Transportr/app/src/test/java/de/grobox/transportr/utils/DateUtilsTest.kt: (82, 43): Unresolved reference: md_green_500
e: /home/pt/code/forks/Transportr/app/src/test/java/de/grobox/transportr/utils/DateUtilsTest.kt: (83, 43): Unresolved reference: md_red_500

@Bnyro
Copy link
Contributor Author

Bnyro commented Jul 31, 2023

Ah, I see!
The colors weren't removed by accident, we just don't use them anymore now.
Removed them from the tests as well, since it doesn't make sense to use them there while the app doesn't use them.

@Bnyro
Copy link
Contributor Author

Bnyro commented Aug 21, 2023

I've been trying to fix the tests, but it turns out they're failing on the master branch as well for me.

Not really sure what that's caused by, any help would be appreciated.

This was referenced Nov 9, 2023
@newhinton
Copy link

@Bnyro Are suggestions welcome?

It is great to see someone tackle the UI migration for Transportr!

@Bnyro
Copy link
Contributor Author

Bnyro commented Jan 8, 2024

@newhinton Sure, feel free to make some proposals. However, due to the fact that I'm quite busy currently I'd also welcome if someone else implemented the improvements and I'll just merge them into my branch if they look fine.

@newhinton
Copy link

I will try to see if i can make them!

Generally, i feel the "border"-centric stlye is a bit heavy to look at. I would love to see a more colorful app, similar to what you did in libretube 0.21. The rest, like the move to "cards" and away from the "blockiness" of material 2 is great and should be kept! I will look if i can do it.

@newhinton
Copy link

newhinton commented Jan 12, 2024

image

@Bnyro I have created a PR to your branch, the above image is an example of a more colorful app!

@Bnyro
Copy link
Contributor Author

Bnyro commented Jan 12, 2024

Thank you! I will look into your changes some time at the beginning of next week, I'm not at home during the weekend unfortunately. If I forget it, please feel free to ping me again :)

@michaelblyons
Copy link

Would that be a good place to use the phone-specific accent color?

@newhinton
Copy link

Would that be a good place to use the phone-specific accent color?

Yes! That's why i am using it ;)

The above screenshot is from an andrid emulator with Android 11 i think, that does not support material you so it defaults back to blue. I should see if i can make the default red again, so that it fits with the original theme better.

@Bnyro
Copy link
Contributor Author

Bnyro commented Jan 31, 2024

closes #927

@Bnyro Bnyro closed this Jan 31, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla-signed ✔️ The Contributor Licence Agreement was signed by all contributors.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants