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 3 DropdownMenu Skiko support #347

Merged
merged 1 commit into from Dec 20, 2022
Merged

Material 3 DropdownMenu Skiko support #347

merged 1 commit into from Dec 20, 2022

Conversation

eygraber
Copy link

@eygraber eygraber commented Dec 8, 2022

Proposed Changes

  • Replace Popup from desktop and uikit with a Popup in skiko
  • Add a DropdownMenu to material3 for skiko

Testing

Test: Copied the DesktopMenuTest from material into material3; kept DesktopPopupTest in ui/desktopTest which now uses Popup from skiko, since I couldn't get the tests to pass in ui/skikoTest (got everything to compile, but couldn't figure out how to idle with ImageComposeScene; maybe related to JetBrains/compose-multiplatform#1866)

Issues Fixed

Addresses JetBrains/compose-multiplatform#2037

Possibly supersedes #328

CLA is signed

// so we need to make additional checks and adjust the position of the [DropdownMenu] to
// avoid content being cut off if the [DropdownMenu] contains too many items.
// See: https://github.com/JetBrains/compose-jb/issues/1388
val popupPositionProvider = SkikoDropdownMenuPositionProvider(
Copy link
Author

Choose a reason for hiding this comment

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

This has diverged from DropdownMenuPositionProvider which now handles LTR. If you want, I can make a followup PR that ports that behavior here and into the material DesktopDropdownMenuPositionProvider.

Copy link
Collaborator

Choose a reason for hiding this comment

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

That would be great, thanks.

P.S. Doesn't it handle LTR already? We have if (layoutDirection == LayoutDirection.Ltr) in the DesktopDropdownMenuPositionProvider/SkikoDropdownMenuPositionProvider

Copy link
Author

Choose a reason for hiding this comment

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

Oh interesting, guess I missed that 😅

@eygraber
Copy link
Author

@igordmn any chance this makes it into 1.3.0?

@igordmn igordmn self-requested a review December 16, 2022 19:52
@igordmn
Copy link
Collaborator

igordmn commented Dec 16, 2022

Hi, thanks for the changes! I will look at your PR's soon.

In the meantime, could you look at this comment, and do what is asked there (sign CLA, put "CLA is signed" in the PR description), and agree on the terms?

@eygraber
Copy link
Author

Updated description regarding CLA and I agree that author fields can be lost

@eygraber
Copy link
Author

Do I need to do this for all my open PRs?

@igordmn
Copy link
Collaborator

igordmn commented Dec 16, 2022

Do I need to do this for all my open PRs?

Yes, just add "CLA is signed" into them

@igordmn igordmn merged commit 94cdbbe into JetBrains:jb-main Dec 20, 2022
@eygraber eygraber deleted the material-3-dropdown branch December 20, 2022 18:51
eymar pushed a commit that referenced this pull request Jan 13, 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
2 participants