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

Expect material3.SearchBar and material3.DockedSearchBar in common #801

Merged
merged 3 commits into from Sep 12, 2023

Conversation

MatkovIvan
Copy link
Member

@MatkovIvan MatkovIvan commented Sep 8, 2023

Proposed Changes

  • Expect material3.SearchBar and material3.DockedSearchBar in common
  • Add test usage in mpp demo app
Screenshot 2023-09-11 at 09 57 16

API Changes

+@ExperimentalMaterial3Api
+@Composable
+fun SearchBar(
+    query: String,
+    onQueryChange: (String) -> Unit,
+    onSearch: (String) -> Unit,
+    active: Boolean,
+    onActiveChange: (Boolean) -> Unit,
+    modifier: Modifier = Modifier,
+    enabled: Boolean = true,
+    placeholder: @Composable (() -> Unit)? = null,
+    leadingIcon: @Composable (() -> Unit)? = null,
+    trailingIcon: @Composable (() -> Unit)? = null,
+    shape: Shape = SearchBarDefaults.inputFieldShape,
+    colors: SearchBarColors = SearchBarDefaults.colors(),
+    tonalElevation: Dp = SearchBarDefaults.Elevation,
+    windowInsets: WindowInsets = SearchBarDefaults.windowInsets,
+    interactionSource: MutableInteractionSource = remember { MutableInteractionSource() },
+    content: @Composable ColumnScope.() -> Unit,
+)
+@ExperimentalMaterial3Api
+@Composable
+fun DockedSearchBar(
+    query: String,
+    onQueryChange: (String) -> Unit,
+    onSearch: (String) -> Unit,
+    active: Boolean,
+    onActiveChange: (Boolean) -> Unit,
+    modifier: Modifier = Modifier,
+    enabled: Boolean = true,
+    placeholder: @Composable (() -> Unit)? = null,
+    leadingIcon: @Composable (() -> Unit)? = null,
+    trailingIcon: @Composable (() -> Unit)? = null,
+    shape: Shape = SearchBarDefaults.dockedShape,
+    colors: SearchBarColors = SearchBarDefaults.colors(),
+    tonalElevation: Dp = SearchBarDefaults.Elevation,
+    interactionSource: MutableInteractionSource = remember { MutableInteractionSource() },
+    content: @Composable ColumnScope.() -> Unit,
+)

Testing

Test: Try to use SearchBar/DockedSearchBar from common or look at mpp demo

Issues Fixed

JetBrains/compose-multiplatform#3528

@MatkovIvan MatkovIvan marked this pull request as ready for review September 11, 2023 08:01
@MatkovIvan MatkovIvan changed the title Expect material3.SearchBar in common Expect material3.SearchBar and material3.DockedSearchBar in common Sep 11, 2023
@@ -449,6 +456,14 @@ private fun SearchBarInputField(
.fillMaxWidth()
.focusRequester(focusRequester)
.onFocusChanged { if (it.isFocused) onActiveChange(true) }
.onKeyEvent {
if (it.key == Key.Escape) {
Copy link

Choose a reason for hiding this comment

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

Better to check whether it's a KEY_PRESSED event; otherwise you could call onActiveChange twice.

Copy link
Member Author

Choose a reason for hiding this comment

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

otherwise you could call onActiveChange twice

The focus will be cleared after first hit, so it's ok to react to any event from Esc

Copy link
Collaborator

@igordmn igordmn Sep 12, 2023

Choose a reason for hiding this comment

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

The focus will be cleared after first hit

It seems that it depends on how onActiveChange implemented by the user?

Copy link
Member Author

Choose a reason for hiding this comment

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

Comment on lines +28 to +33
val density = LocalDensity.current
val windowInfo = LocalWindowInfo.current
return with(density) {
windowInfo.containerSize.height.toDp()
}
}
Copy link

Choose a reason for hiding this comment

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

Very minor stylistic nit, but maybe better looking without the local variables:

return with(LocalDensity.current) {
    LocalWindowInfo.current.containerSize.height.toDp()
}

Copy link
Member Author

Choose a reason for hiding this comment

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

Well, it was my initial implementation. But I introduced var for better looking 😄

@MatkovIvan MatkovIvan merged commit 4ec6383 into jb-main Sep 12, 2023
3 of 4 checks passed
@MatkovIvan MatkovIvan deleted the ivan.matkov/common-searchbar3 branch September 12, 2023 14:49
MatkovIvan added a commit that referenced this pull request Sep 13, 2023
## Proposed Changes

Addition to #801

## Testing

Test: Don't change the state in `onActiveChange`, press `Esc` - it was
called twice
igordmn pushed a commit that referenced this pull request Nov 15, 2023
…r` in common (#801)

* Move SearchBar to common

* Handle platform specifics

* Add search bar sample
igordmn pushed a commit that referenced this pull request Nov 15, 2023
## Proposed Changes

Addition to #801

## Testing

Test: Don't change the state in `onActiveChange`, press `Esc` - it was
called twice
igordmn pushed a commit that referenced this pull request Nov 16, 2023
…material3.DockedSearchBar` in common (#801)

* Move SearchBar to common

* Handle platform specifics

* Add search bar sample
igordmn pushed a commit that referenced this pull request Jan 30, 2024
#801)

* Move SearchBar to common

* Handle platform specifics

* Add search bar sample
igordmn pushed a commit that referenced this pull request Jan 30, 2024
## Proposed Changes

Addition to #801

## Testing

Test: Don't change the state in `onActiveChange`, press `Esc` - it was
called twice
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants