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

Fixes Dropdown wrong default color #40

Closed
wants to merge 5 commits into from
Closed

Conversation

MV-GH
Copy link

@MV-GH MV-GH commented Aug 27, 2023

According to the m3 specs, dropdown has a 3dp tonal elevation, this was missing and caused the discrepancy between DropdownMenu and CascadeDropdownMenu. See https://m3.material.io/styles/elevation/tokens#df721a00-888e-4c5e-bfe1-5d905f167aaa

After:

8Z8ul9eEY3.mp4

Before:

studio64_ydSz9zlk9O.mp4

Fixes #39

Copy link
Owner

@saket saket left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you so much!

@saket
Copy link
Owner

saket commented Aug 27, 2023

Can you regenerate test screenshots using and add them to this PR?

./gradlew cascade-compose:connectedDebugAndroidTest -Pdropshots.record

@MV-GH
Copy link
Author

MV-GH commented Aug 27, 2023

I generated them using a Pixel 7 API 31.

@MV-GH
Copy link
Author

MV-GH commented Aug 29, 2023

Made some small changes, I saw that it was setting the shadowElevation to the tonalElevation. And made a argument for tonalElevation

@MV-GH
Copy link
Author

MV-GH commented Aug 29, 2023

There was a test with this remark

// Drop shadows are strangely causing tiny differences in
// generated screenshots. A zero elevation is used to avoid this.
private val shadowElevation = 0.dp

This was actually changing the tonalElevation and now it actually changes the shadowElevation

Not sure if this is still needed?

@saket
Copy link
Owner

saket commented Oct 4, 2023

I'm sorry this took me some time. trunk has moved ahead so let me land your changes manually.

@@ -118,6 +119,7 @@ fun CascadeDropdownMenu(
offset: DpOffset = DpOffset.Zero,
fixedWidth: Dp = 196.dp,
shadowElevation: Dp = 3.dp,
tonalElevation: Dp = 3.dp,
Copy link
Owner

@saket saket Oct 4, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you need your popups to have a different shadow and tonal elevations?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't, but others might and this matches the Compose API as it gives both options

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@saket saket closed this Oct 8, 2023
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.

The default color of the CascadeDropdownMenu is not the same as DropdownMenu
2 participants