-
Notifications
You must be signed in to change notification settings - Fork 74
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
Refactor UIKit interop implementation #1411
Conversation
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.
Just a minor feedback regarding naming. LGTM overall
EmptyLayout( | ||
modifier.onGloballyPositioned { coordinates -> | ||
val finalModifier = modifier |
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.
It was easier without extra val
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.
Way too much indentation and it's an idiom used in the common source set as well
Lines 648 to 669 in 7a13310
val coreTextFieldModifier = Modifier | |
// min height is set for maxLines == 1 in order to prevent text cuts for single line | |
// TextFields | |
.heightIn(min = state.minHeightForSingleLineField) | |
.heightInLines( | |
textStyle = textStyle, | |
minLines = minLines, | |
maxLines = maxLines | |
) | |
.overscroll() | |
.textFieldScroll( | |
scrollerPosition = scrollerPosition, | |
textFieldValue = value, | |
visualTransformation = visualTransformation, | |
textLayoutResultProvider = { state.layoutResult }, | |
) | |
.then(cursorModifier) | |
.then(drawModifier) | |
.textFieldMinSize(textStyle) | |
.then(onPositionedModifier) | |
.then(magnifierModifier) | |
.bringIntoViewRequester(bringIntoViewRequester) |
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.
indentation is the same, isn't it?
compose/ui/ui/src/uikitMain/kotlin/androidx/compose/ui/interop/UIKitView.uikit.kt
Outdated
Show resolved
Hide resolved
I'll do some more stuff along this PR |
1a15002
to
ea777e1
Compare
c0a212b
to
9859fff
Compare
compose/ui/ui/src/skikoMain/kotlin/androidx/compose/ui/node/InteropContainer.skiko.kt
Show resolved
Hide resolved
compose/ui/ui/src/uikitMain/kotlin/androidx/compose/ui/interop/UIKitInteropContainer.uikit.kt
Outdated
Show resolved
Hide resolved
@@ -38,35 +40,124 @@ internal val LocalUIKitInteropContainer = staticCompositionLocalOf<UIKitInteropC | |||
error("UIKitInteropContainer not provided") | |||
} | |||
|
|||
|
|||
internal enum class UIKitInteropState { |
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.
IMHO, interop
and enum values are too generic.
Could you please add brief comment explaining why this enum used?
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.
Yeah, sure
Refactor UIKit interop implementation:
UIKitView
andUIKitViewController
common part toUIKitInteropLayout
.InteropComponentHandler
.UIKitInteropContext
intoUIKitInteropContainer
.