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

Configure the "material3-window-size-class" module as CMP and include it to the publication #1466

Merged
merged 5 commits into from
Jul 26, 2024

Conversation

terrakok
Copy link
Member

Configure the material3-window-size-class module as CMP and include it to the publication.
Added new method to non-android source set to calculate WindowSizeClass.

Fixes https://youtrack.jetbrains.com/issue/CMP-2404

Testing

Checked it in the MPP demo app:
image

Release Notes

Features - Multiple Platforms

  • Commonized material3-window-size-class module

@terrakok terrakok requested a review from MatkovIvan July 25, 2024 17:10
Copy link
Member

@MatkovIvan MatkovIvan left a comment

Choose a reason for hiding this comment

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

Please move WindowSizeClassTest to commonTest it should work almost as-is.

Also please generate API dump (reason of red CI)

Comment on lines +121 to +135
skikoMain.dependsOn(commonMain)
desktopMain.dependsOn(skikoMain)
nonJvmMain.dependsOn(skikoMain)
webMain.dependsOn(nonJvmMain)
jsMain.dependsOn(webMain)
wasmJsMain.dependsOn(webMain)
nativeMain.dependsOn(nonJvmMain)

skikoTest.dependsOn(commonTest)
desktopTest.dependsOn(skikoTest)
nonJvmTest.dependsOn(skikoTest)
webTest.dependsOn(nonJvmTest)
jsTest.dependsOn(webTest)
wasmJsTest.dependsOn(webTest)
nativeTest.dependsOn(nonJvmTest)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
skikoMain.dependsOn(commonMain)
desktopMain.dependsOn(skikoMain)
nonJvmMain.dependsOn(skikoMain)
webMain.dependsOn(nonJvmMain)
jsMain.dependsOn(webMain)
wasmJsMain.dependsOn(webMain)
nativeMain.dependsOn(nonJvmMain)
skikoTest.dependsOn(commonTest)
desktopTest.dependsOn(skikoTest)
nonJvmTest.dependsOn(skikoTest)
webTest.dependsOn(nonJvmTest)
jsTest.dependsOn(webTest)
wasmJsTest.dependsOn(webTest)
nativeTest.dependsOn(nonJvmTest)
skikoMain.dependsOn(commonMain)
targets.configureEach { target ->
if (target.platformType !in [KotlinPlatformType.androidJvm, KotlinPlatformType.common]) {
target.compilations["main"].defaultSourceSet {
dependsOn(skikoMain)
}
target.compilations["test"].defaultSourceSet {
dependsOn(commonTest)
}
}
}

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't see a profit of this code. it is harder to understand and spends same number of lines of code

Copy link
Member

Choose a reason for hiding this comment

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

Because you need to manually add dependencies to nativeMain to all specific architectures/OSes

Copy link
Member Author

@terrakok terrakok Jul 26, 2024

Choose a reason for hiding this comment

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

I wouldn't say I agree with the proposed changes.
To have a flexible and common logic for all modules we need to extract it to the plugin. At the moment I see just copy-pasted code snippets around the project.

Copy link
Member Author

@terrakok terrakok Jul 26, 2024

Choose a reason for hiding this comment

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

your snippet adds every leaf sources as dependy to the skiko. why? we need hierarhical structure
image

@terrakok terrakok requested a review from MatkovIvan July 26, 2024 10:53
@terrakok terrakok merged commit 16f1e84 into jb-main Jul 26, 2024
6 checks passed
@terrakok terrakok deleted the k.tskh/material3-window-size-class branch July 26, 2024 13:53
mazunin-v-jb pushed a commit that referenced this pull request Aug 14, 2024
… it to the publication (#1466)

Configure the `material3-window-size-class` module as CMP and include it
to the publication.
Added new method to non-android source set to calculate
`WindowSizeClass`.

Fixes https://youtrack.jetbrains.com/issue/CMP-2404

## Testing
Checked it in the MPP demo app:
<img width="400" alt="image"
src="https://github.com/user-attachments/assets/4c1b6fab-132d-4d1f-91fb-2c5a14bd9cab">


## Release Notes
### Features - Multiple Platforms
- Commonized `material3-window-size-class` module
@chrisjenx
Copy link

Was it intentional to not add this too the ComposePlugin ?

@FunkyMuse
Copy link

Was it intentional to not add this too the ComposePlugin ?

I'm wondering the same thing

in the meantime

jetbrains-compose-window-size = { module = "org.jetbrains.compose.material3:material3-window-size-class", version.ref = "cmp-plugin" }

@chrisjenx
Copy link

Yes but it doesn't work so I wouldn't worry. Not sure how the tests passed TBH..

@FunkyMuse
Copy link

FunkyMuse commented Aug 21, 2024

Yes but it doesn't work so I wouldn't worry. Not sure how the tests passed TBH..

Yeah, same here, I can't seem to reference the function calculateWindowSizeClass
Just wrote to slack https://kotlinlang.slack.com/archives/C3PQML5NU/p1724255794662019

@terrakok
Copy link
Member Author

terrakok commented Aug 22, 2024

Pull requests are not right place to discuss features or issues. Let's do this on the youtrack

For your case:

val windowSizeClass = currentWindowAdaptiveInfo().windowSizeClass

@JetBrains JetBrains locked and limited conversation to collaborators Aug 22, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants