-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
Implement Material 3 "pill" component #5765
Conversation
@seadowg - looking good! I think the spacing is working well - it doesn't feel too squished. I think it will be easier to scan when we remove the form icon, but that can come later. A few design tweaks: "Complete" pill
"Incomplete" Pill
|
@alyblenkin this is good for another look! The colors had to be hacked in as this release isn't using Material 3 yet so I we should revise this again when we merge into v2023.4. |
Looks great! The blue I'm using in figma must be darker because the grey pill really stands out here. Let's make the grey lighter so the contrast isn't so stark between the two. Could we try #ECECEC or the grey we use for the main menu buttons? |
@alyblenkin I don't why I didn't just use that color to begin with actually! I've updated it. |
material/src/main/java/org/odk/collect/material/MaterialPill.kt
Outdated
Show resolved
Hide resolved
<vector android:height="24dp" android:tint="#000000" | ||
android:viewportHeight="24" android:viewportWidth="24" | ||
android:width="24dp" xmlns:android="http://schemas.android.com/apk/res/android"> | ||
<path android:fillColor="@android:color/white" android:pathData="M9,16.17L4.83,12l-1.42,1.41L9,19 21,7l-1.41,-1.41z"/> |
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.
If the tint color is black it doesn't make sense to use white here right it's confusing. It should be black in both 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.
This is actually something I realise I'm really unclear on. Given we might have an icon appear in multiple places where it needs different tints, should we have multiple vector assets with different tints or just have one that's a default black (or probably ?colorOnSurface
)? It seems from this and the other comments you'd lean towards the former?
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 if it's used in only one place like here we can directly set the only color that's used in the XML file and don't think about it but generally yeah I think the first option is what I prefer.
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.
Ah I think I always need to set it programmatically anyway as otherwise I can't have a default tint
in pill.xml
as it'll always override the one in the drawable. For now, I'll keep the setIconTint
calls and make the icons use ?colorOnSurface
as their default (which makes more sense than hardcoding black I think). Let me know if that doesn't sound right to you!
I started testing and there was a crash after tapping "Finalize all forms" in a form with pull data. Is PR #5775 included in CircleCi apk? |
@dbemke I've rebased so the latest changes from |
It seems that something changed in the dark mode. In the end screen "You will not be able to make edits..." has the light mode background although in the app the dark mode. I checked if it's connected with opposite modes device vs app but this doesn't seem to be the case (device dark mode, app dark mode- the issue occurs). I also checked the store version and I wasn't able to reproduce it there. |
@dbemke that's fixed! |
@dbemke does it occur on |
I checked 2023.3.x 07d4688 and it occurs there too. So I'll file a separate issue. |
Tested with Success! Verified on device with Android 10 Verified cases:
|
Tested with Success! Verified on device with Android 13 |
Implement Material 3 "pill" component
Implement Material 3 "pill" component
Blocked by #5760The "pill" component appears to exist in several places in Material 3's own spec (this table for instance), but doesn't seem to have been explicitly called out or implemented. It also appears in Google Maps.
This PR provides an implementation of the component that should adapt appropriately to shape, typography and color theming.
What has been done to verify that this works as intended?
Verified manually.
Why is this the best possible solution? Were any other approaches considered?
For code reviewers: the shape theming was the most complex here and involved some reverse engineering of the implementation of several Material components. In short, I ended up building a
ShapeAppearanceModel
with the theme'sshapeAppearanceCornerSmall
attribute and then used that to create aMaterialShapeDrawable
that I could change the color on and set as the component's background. I'm not 100% sure this is correct™️, so it'd be worth doing some quick research to see if you agree with me. There are some docs on this (here), but they seemed to be more aimed at customising the shapes at a theme level and I didn't personally get much from them.How does this change affect users? Describe intentional changes to behavior and behavior that could have accidentally been affected by code changes. In other words, what are the regression risks?
This is just a visual change to the "complete" and "incomplete" pill on drafts, so verifying that they look correct is all that's needed really.
Before submitting this PR, please make sure you have:
./gradlew checkAll
and confirmed all checks still pass OR confirm CircleCI build passes and run./gradlew connectedDebugAndroidTest
locally.