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

MD3 native dropdown support #328

Closed
wants to merge 2 commits into from

Conversation

blazsolar
Copy link

Adding dropdown support for native target in material 3 library.

CLA is signed

Copy link
Collaborator

@igordmn igordmn left a comment

Choose a reason for hiding this comment

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

Thanks!

onDismissRequest = onDismissRequest,
popupPositionProvider = popupPositionProvider,
onPreviewKeyEvent = onPreviewKeyEvent,
onKeyEvent = onKeyEvent,
Copy link
Collaborator

Choose a reason for hiding this comment

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

DesktopMenu.desktop.kt has additional logic here. Let's copy it here too.

* [Interaction]s and customize the appearance / behavior of this menu item in different states.
*/
@Composable
fun DropdownMenuItem(
Copy link
Collaborator

Choose a reason for hiding this comment

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

Better to not commonize public functions without commonizing them in the original androidx repository first. It can cause issues. Let's just copy it to uikitMain for now.

Commonization of DropdownMenu is a separate issue

modifier: Modifier = Modifier,
offset: DpOffset = DpOffset(0.dp, 0.dp),
onPreviewKeyEvent: ((KeyEvent) -> Boolean) = { false },
onKeyEvent: ((KeyEvent) -> Boolean) = { false },
Copy link
Collaborator

Choose a reason for hiding this comment

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

We probably need to add these parameters to another implementations first, and only then here. The order of parameters may change (because of preserving backward compatibility for the old implementations)

If you don't need them, let's not add them for now.

@blazsolar
Copy link
Author

@igordmn Looks like #347 addresses all the comments above. I guess we can close this PR in favor of #347

@igordmn
Copy link
Collaborator

igordmn commented Dec 20, 2022

@igordmn Looks like #347 addresses all the comments above. I guess we can close this PR in favor of #347

Yes, #347 resolved the issues. Let's close this PR. Thanks for the contribution anyway :)

@igordmn igordmn closed this Dec 20, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants