-
Notifications
You must be signed in to change notification settings - Fork 80
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
Kotlinx datetime CalendarModel (support date picker on darwin and web) #717
Conversation
compose/material3/material3/src/skikoMain/kotlin/androidx/compose/material3/Strings.skiko.kt
Show resolved
Hide resolved
...erial3/src/desktopTest/kotlin/androidx/compose/material3/KotlinxDatetimeCalendarModelTest.kt
Outdated
Show resolved
Hide resolved
...erial3/src/desktopTest/kotlin/androidx/compose/material3/KotlinxDatetimeCalendarModelTest.kt
Outdated
Show resolved
Hide resolved
I am not sure how multiplatform tests work. Need some help here.
|
Unless actual implementations intentionally produce different results, there is no need to add the same test to platform-specific test source sets. skikoTests should be fine. |
...ial3/material3/src/jsWasmMain/kotlin/androidx/compose/material3/PlatformDateFormat.jsWasm.kt
Outdated
Show resolved
Hide resolved
...ial3/material3/src/jsWasmMain/kotlin/androidx/compose/material3/PlatformDateFormat.jsWasm.kt
Outdated
Show resolved
Hide resolved
...ial3/material3/src/jsWasmMain/kotlin/androidx/compose/material3/PlatformDateFormat.jsWasm.kt
Outdated
Show resolved
Hide resolved
...erial3/src/desktopTest/kotlin/androidx/compose/material3/KotlinxDatetimeCalendarModelTest.kt
Outdated
Show resolved
Hide resolved
Should we use new calendar model for desktop too and set it as actual in skiko? Or it's better to left legacy one for now? Date format is delegated to legacy anyway. |
...ial3/material3/src/jsWasmMain/kotlin/androidx/compose/material3/PlatformDateFormat.jsWasm.kt
Outdated
Show resolved
Hide resolved
If the desktop tests pass, then I think it's better to use a common implementation (common for skiko targets without android). And as for formatter, let it delegate to legacy impl for now. Maybe adding a link to Kotlin/kotlinx-datetime#251 in desktop formatter could be helpful in the future. |
Co-authored-by: Oleksandr Karpovich <[email protected]>
They do. Except 2 muted (only on desktop) tests for a non-implemented desktop ICU skeleton feature. |
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.
Web part looks good to me! thank you!
It is a consequence of non-implemented ICU date skeletons. We can replace it for now with something readable without localization (for ex. take patterns from the left screen). The only thing there can be troubles c падежами in some languages. |
compose/material3/material3/src/skikoTest/kotlin/androidx/compose/material3/Assert.kt
Outdated
Show resolved
Hide resolved
Co-authored-by: dima.avdeev <[email protected]>
@alexzhirkevich We added automatically check Google CLA. You need to add an additional email (in GitHub account) to which you accepted the signature |
Yeah, thanks! Overall LGTM. I will merge it |
#717) implement Calender on all platforms Co-authored-by: Oleksandr Karpovich <[email protected]> Co-authored-by: dima.avdeev <[email protected]>
Proposed Changes
String.format
KotlinxDatetimeCalendarModel
macOS native
Testing
Test:
KotlinxDatetimeCalendarModelTest
+ mpp Date & Time pickersIssues Fixed
Fixes: JetBrains/compose-multiplatform#3359
Fixes: JetBrains/compose-multiplatform#3354
Fixes: JetBrains/compose-multiplatform#1299