-
-
Notifications
You must be signed in to change notification settings - Fork 188
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 (V2) #927
base: master
Are you sure you want to change the base?
Conversation
use original color scheme
improve material 3 shading
Thanks @newhinton and @Bnyro! The screenshots in the linked PR already look very promising! I will try to test your changes in the coming days and provide some initial feedback. |
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.
Hi @newhinton and @Bnyro, I've now gone through the code and left some comments below. Thanks again for this work!
.idea/codeStyles/Project.xml
Outdated
<XML> | ||
<option name="XML_LEGACY_SETTINGS_IMPORTED" value="true" /> | ||
</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.
This looks like some unrelated changes which should be left out of the PR.
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.
@ialokim I have never (manually) touched those files. Should IDE-config files be in this repository anyhow?
KeyboardUtil.hideKeyboard(activity) | ||
val imm = activity?.getSystemService(Activity.INPUT_METHOD_SERVICE) as InputMethodManager? | ||
imm?.hideSoftInputFromWindow(requireActivity().currentFocus?.windowToken, 0) |
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 would vote for having our own KeyboardUtil.kt
that encapsulates this rather ugly piece of code.
InputMethodManager imm = (InputMethodManager) getSystemService(Activity.INPUT_METHOD_SERVICE); | ||
View currentFocus = getCurrentFocus(); | ||
if (currentFocus != null) imm.hideSoftInputFromWindow(currentFocus.getWindowToken(), 0); |
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.
As before, I would move this to KeyboardUtils
.
FullScreenUtil.Companion.drawBehindStatusbar(this); | ||
FullScreenUtil.Companion.applyTopInset(findViewById(R.id.searchCardView), 16); |
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.
This misbehaves on my device, see screenshots.
app/src/main/java/de/grobox/transportr/networks/PickTransportNetworkActivity.kt
Show resolved
Hide resolved
android:layout_marginTop="32dp" | ||
android:fitsSystemWindows="true"> | ||
|
||
<LinearLayout |
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.
Couldn't we use a ConstrainedLayout
instead of nested LinearLayout
s?
|
||
<TextView | ||
android:id="@+id/via" | ||
android:layout_width="0dp" | ||
android:layout_height="wrap_content" | ||
android:ellipsize="middle" | ||
android:singleLine="true" | ||
android:textColor="?android:textColorPrimary" |
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.
As said before, not a fan of having Material You-adapted text colors.
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 name="md_theme_primary">@android:color/system_accent1_200</color> | ||
<color name="md_theme_onPrimary">@android:color/system_accent1_0</color> |
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.
Where did you get these values from? Some online tool?
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.
</style> | ||
|
||
<style name="AppDayNightTheme" parent="AppBaseTheme"> | ||
<item name="colorControlNormal">@color/drawableTintLight</item> |
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.
Shouldn't this still be set somewhere or is it provided by MD3 somehow automatically? (You've used it in several places)
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.
It's automatically set by Android
`when`(resources.getColor(R.color.md_green_500)).thenReturn(GREEN) | ||
`when`(resources.getColor(R.color.md_red_500)).thenReturn(RED) |
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.
Have you tried that the test still runs through?
I haven't encountered that one.
But those ones yes.
Given this is not a regression, I wouldn't further consider it for this PR.
This one still happens here. |
This definitely needs some more cleanup work, but I do think this is going into the right direction! Personally, I'm not fully convinced on giving up on the nice Transportr red altogether, given that it's part of the branding, but it seems that's what Material You is suggesting to do? |
Yes, it does. It would be possible to add s preference to toggle the red vs material you colors though. |
+1 for trying to keep the Transportr red identity by default if possible :) |
I think this is what Trail Sense is doing. |
This is actually not a regression, it is also present in the current design. It is just a bit more visible in this PR.
Yes it is. Though i recommend not to introduce a toggle ability, it would require quite a bit of code to make it work properly. It will lead to bugs. And i don't think i want to spend time on implementing that, especially since there is still a bit to do with the base-proposal. |
Todo:
i dont think its a Todo:
|
I have improved on many of the things you noted. However, there are still some things broken, like the three-state-bottomsheet. I also found that some issues are very likely due to older libraries beeing used. That opened an entire different can of worms. As an example: I have tried to manually calculate the statusbar height so that i can adjust the backButton in the tripview, with less than optimal results. In newer libraries there is an This is rapidly exploding the size of this PR, but i also don't think that we should continue without upgrading the libraries, which are at this point 2-4 years old and up to 5 versions behind. How should i continue? |
I think #902 is a step in the right direction regarding the issues you mentionned. We just need to review/merge it :) |
Closes #872
This PR is based on, and superseeds @Bnyro 's Material 3 implementation. It takes the UI a step further, and makes heavy use of the Material You colors.
This was implemented and improved with @Bnyro support, consent and collaboration.
Edit:
For more info about what this pr does, look in this discussion: https://github.com/Bnyro/Transportr/pull/1