Conversation
…ial PR #20 with architectural improvements, documentation, and stability fixes (#70) * Initial plan * Add simple unified wrapper components: RadioButton, Divider, Slider, Badge Co-authored-by: ShreckYe <27768951+ShreckYe@users.noreply.github.com> * Add Chips, Dialog, and Tabs unified wrapper components Co-authored-by: ShreckYe <27768951+ShreckYe@users.noreply.github.com> * Add SegmentedButton labs components Co-authored-by: ShreckYe <27768951+ShreckYe@users.noreply.github.com> * Add remaining unified wrapper components: Select, ListItem, NavigationDrawer Co-authored-by: ShreckYe <27768951+ShreckYe@users.noreply.github.com> * Add new components to Material3 demo Co-authored-by: ShreckYe <27768951+ShreckYe@users.noreply.github.com> * Fix compilation errors * Fix demo: RadioButton with Selection enum, Badge with TaglessText, Chips with separate states, improved Slider Co-authored-by: ShreckYe <27768951+ShreckYe@users.noreply.github.com> * Remove ListItem component, use existing ListScope.ListItem implementation Co-authored-by: ShreckYe <27768951+ShreckYe@users.noreply.github.com> * Fix SegmentedButton: use Selection enum, add multi-select demo, improve label rendering Co-authored-by: ShreckYe <27768951+ShreckYe@users.noreply.github.com> * Refactor to Compose UI naming: Single/MultiChoiceSegmentedButtonRow, move components out of labs, add Select to ext package Co-authored-by: ShreckYe <27768951+ShreckYe@users.noreply.github.com> * Update copilot-instructions.md with Material 3 component list and architectural patterns Co-authored-by: ShreckYe <27768951+ShreckYe@users.noreply.github.com> * Fix segmented row scope API, composeUi segmented rendering, and ext select behavior; resolve JS webpack segmented import issue Co-authored-by: ShreckYe <27768951+ShreckYe@users.noreply.github.com> * Remove the second `List` component added by Copilot * Review and incompletely fix changes in commit 9ec4b94 * Update README Material 3 API catalog to match implemented components Co-authored-by: ShreckYe <27768951+ShreckYe@users.noreply.github.com> * Add m3.material.io component lookup note to copilot instructions Co-authored-by: ShreckYe <27768951+ShreckYe@users.noreply.github.com> --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: ShreckYe <27768951+ShreckYe@users.noreply.github.com> Co-authored-by: Yongshun Ye <ShreckYe@gmail.com>
|
Summary of ChangesHello @ShreckYe, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly expands the library's Material 3 component offerings, providing a wider range of unified UI elements for multiplatform applications. Concurrently, it updates core build dependencies and configurations to ensure compatibility and improve development experience, while formally deprecating and removing Material 2 components to align with modern design standards. These changes aim to enhance the library's feature set and maintain its technical foundation. Highlights
Changelog
Ignored Files
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request adds a significant number of new unified Material 3 components, along with their expect/actual implementations for Compose UI and JS platforms, and updates the demo application and documentation to reflect these changes. While this is a great step forward, several of the new component implementations are incomplete or have critical bugs, especially on the Compose UI side. There are multiple TODOs left in the code that render components unusable, incorrect implementations that don't match the component's purpose, and missing functionality. I've left specific comments on these issues.
| actual fun ModalNavigationDrawer( | ||
| opened: Boolean, | ||
| onDismissRequest: () -> Unit, | ||
| modifier: Modifier, | ||
| content: @Composable () -> Unit | ||
| ) = | ||
| androidx.compose.material3.ModalNavigationDrawer( | ||
| drawerContent = { content() }, | ||
| modifier = modifier.platformModifier, | ||
| drawerState = androidx.compose.material3.rememberDrawerState( | ||
| if (opened) androidx.compose.material3.DrawerValue.Open | ||
| else androidx.compose.material3.DrawerValue.Closed | ||
| ), | ||
| gesturesEnabled = true | ||
| ) { | ||
| // Empty content for the drawer | ||
| } |
There was a problem hiding this comment.
The composeUi implementation of ModalNavigationDrawer is critically flawed. It uses the content parameter for the drawerContent, but leaves the main content block of the underlying androidx.compose.material3.ModalNavigationDrawer empty. This will result in the drawer appearing over a blank screen. To fix this, the expect declaration for ModalNavigationDrawer should be updated to accept both drawerContent and a main content lambda, and both should be passed to the actual implementation.
| @Composable | ||
| actual fun SegmentedButton( | ||
| selected: Boolean, | ||
| onClick: () -> Unit, | ||
| modifier: Modifier, | ||
| enabled: Boolean, | ||
| icon: @Composable (() -> Unit)?, | ||
| label: @Composable () -> Unit | ||
| ) = | ||
| platformScope.SegmentedButton( | ||
| selected, | ||
| onClick, | ||
| SegmentedButtonDefaults.itemShape( | ||
| index = TODO(), | ||
| count = TODO() | ||
| ), | ||
| modifier.platformModifier, | ||
| enabled, | ||
| icon = TODO(), | ||
| label = label | ||
| ) |
There was a problem hiding this comment.
The composeUi implementation of SegmentedButton within SingleChoiceSegmentedButtonRowScope is incomplete and incorrect. It has TODO() placeholders for index, count, and icon, which are essential for the component to function and render correctly. The call to platformScope.SegmentedButton is also incorrect as platformScope is a RowScope. This makes the SingleChoiceSegmentedButtonRow component unusable on Compose UI platforms.
| @Composable | ||
| actual fun SegmentedButton( | ||
| selected: Boolean, | ||
| onClick: () -> Unit, | ||
| modifier: Modifier, | ||
| enabled: Boolean, | ||
| icon: @Composable (() -> Unit)?, | ||
| label: @Composable () -> Unit | ||
| ) = | ||
| TODO() as Unit | ||
| } |
There was a problem hiding this comment.
The SegmentedButton implementation within MultiChoiceSegmentedButtonRowScope is completely missing and is just a TODO(). This renders the MultiChoiceSegmentedButtonRow component non-functional on Compose UI platforms.
| actual fun NavigationDrawer( | ||
| opened: Boolean, | ||
| modifier: Modifier, | ||
| content: @Composable () -> Unit | ||
| ) { | ||
| // TODO: Implement using Material 3 PermanentNavigationDrawer or similar | ||
| // For now, just render content if opened | ||
| if (opened) { | ||
| content() | ||
| } | ||
| } |
There was a problem hiding this comment.
The composeUi implementation of NavigationDrawer is incomplete. It currently only conditionally renders its content based on the opened flag, which doesn't match the behavior of a standard navigation drawer. The TODO comment acknowledges that it should be implemented using PermanentNavigationDrawer or a similar component. As it stands, this implementation is misleading and doesn't provide the expected UI pattern.
| actual fun OutlinedSelect( | ||
| value: String, | ||
| onValueChange: (String) -> Unit, | ||
| modifier: Modifier, | ||
| enabled: Boolean, | ||
| label: String?, | ||
| options: @Composable () -> Unit | ||
| ) = | ||
| // TODO use `ExposedDropdownMenuBoxWithOutlinedTextField` | ||
| FilledSelect(value, onValueChange, modifier, enabled, label, options) |
There was a problem hiding this comment.
The OutlinedSelect component is incorrectly implemented as it just delegates to FilledSelect. This means it will render as a filled select instead of an outlined one, which is misleading. The TODO comment indicates this is known, but it's a significant issue that should be addressed.
// TODO use `ExposedDropdownMenuBoxWithOutlinedTextField`
// For now, this is a placeholder. It should be implemented with an OutlinedTextField.
// Example:
/*
var expanded by remember { mutableStateOf(false) }
ExposedDropdownMenuBox(expanded = expanded, onExpandedChange = { expanded = !expanded }) {
OutlinedTextField(
value = value,
onValueChange = {},
readOnly = true,
label = { Text(label ?: "") },
trailingIcon = { ExposedDropdownMenuDefaults.TrailingIcon(expanded = expanded) },
modifier = Modifier.menuAnchor()
)
ExposedDropdownMenu(
expanded = expanded,
onDismissRequest = { expanded = false }
) {
options()
}
}
*/
FilledSelect(value, onValueChange, modifier, enabled, label, options)| actual fun AlertDialog( | ||
| onDismissRequest: () -> Unit, | ||
| confirmButton: @Composable () -> Unit, | ||
| modifier: Modifier, | ||
| dismissButton: @Composable (() -> Unit)?, | ||
| icon: @Composable (() -> Unit)?, | ||
| title: @Composable (() -> Unit)?, | ||
| text: @Composable (() -> Unit)? | ||
| ) { | ||
| var isOpen by remember { mutableStateOf(true) } | ||
|
|
||
| if (isOpen) { | ||
| MdDialog( | ||
| open = true, | ||
| attrs = modifier.toAttrs() | ||
| ) { | ||
| title?.let { titleContent -> | ||
| Div({ | ||
| slotEqHeadline() | ||
| }) { | ||
| icon?.let { iconContent -> iconContent() } | ||
| titleContent() | ||
| } | ||
| } | ||
|
|
||
| text?.let { textContent -> | ||
| Div({ | ||
| slotEqContent() | ||
| }) { | ||
| textContent() | ||
| } | ||
| } | ||
|
|
||
| Div({ | ||
| slotEqActions() | ||
| }) { | ||
| dismissButton?.let { dismissBtn -> dismissBtn() } | ||
| confirmButton() | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
The JS implementation of AlertDialog has a few issues:
- The
onDismissRequestlambda is ignored. This means the dialog cannot be dismissed by clicking the background scrim or pressing the Escape key, which is standard behavior. - It uses an internal
isOpenstate, which is unnecessary as the component's presence in the composition is already controlled externally (as seen in the demo).
To fix this, you should remove the internal state and use an event listener for the closed event on MdDialog to trigger onDismissRequest.
actual fun AlertDialog(
onDismissRequest: () -> Unit,
confirmButton: @Composable () -> Unit,
modifier: Modifier,
dismissButton: @Composable (() -> Unit)?,
icon: @Composable (() -> Unit)?,
title: @Composable (() -> Unit)?,
text: @Composable (() -> Unit)?
) {
MdDialog(
open = true,
attrs = modifier.toAttrs {
addEventListener("closed") { onDismissRequest() }
}
) {
title?.let { titleContent ->
Div({
slotEqHeadline()
}) {
icon?.let { iconContent -> iconContent() }
titleContent()
}
}
text?.let { textContent ->
Div({
slotEqContent()
}) {
textContent()
}
}
Div({
slotEqActions()
}) {
dismissButton?.let { dismissBtn -> dismissBtn() }
confirmButton()
}
}
}| var selectedValue by remember { mutableStateOf("Option 1") } | ||
| Column { | ||
| Text("Selected: $selectedValue") | ||
| FilledSelect( | ||
| value = selectedValue, | ||
| onValueChange = { selectedValue = it }, | ||
| label = "Choose option" | ||
| ) { | ||
| SelectOption("Option 1", "Option 1") | ||
| SelectOption("Option 2", "Option 2") | ||
| SelectOption("Option 3", "Option 3") | ||
| } | ||
| } | ||
| Column { | ||
| OutlinedSelect( | ||
| value = selectedValue, | ||
| onValueChange = { selectedValue = it }, | ||
| label = "Choose option" | ||
| ) { | ||
| SelectOption("Option 1", "Option 1") | ||
| SelectOption("Option 2", "Option 2") | ||
| SelectOption("Option 3", "Option 3") | ||
| } | ||
| } |
There was a problem hiding this comment.
In the demo for Select components, you have a FilledSelect and an OutlinedSelect one after another, both bound to the same selectedValue state. While this works, for better demonstration purposes, consider placing them side-by-side in a Row and adding Text labels to clarify which is which. This would make the demo clearer and easier to understand visually.
| // TODO check their implementations and consider unifying their duplicate implementations | ||
|
|
||
| actual class SingleChoiceSegmentedButtonRowScope(val elementScope: ElementScope<HTMLElement>) { | ||
| @MaterialWebLabsApi | ||
| @Composable | ||
| actual fun SegmentedButton( | ||
| selected: Boolean, | ||
| onClick: () -> Unit, | ||
| modifier: Modifier, | ||
| enabled: Boolean, | ||
| icon: @Composable (() -> Unit)?, | ||
| label: @Composable () -> Unit | ||
| ) { | ||
| MdOutlinedSegmentedButton( | ||
| disabled = enabled.isFalseOrNull(), | ||
| selected = selected.isTrueOrNull(), | ||
| hasIcon = (icon != null).isTrueOrNull(), | ||
| attrs = modifier.toAttrs { | ||
| this.onClick { onClick() } | ||
| } | ||
| ) { | ||
| icon?.let { iconContent -> | ||
| Div({ | ||
| slotEqIcon() | ||
| }) { | ||
| iconContent() | ||
| } | ||
| } | ||
| // Render label in the default slot | ||
| label() | ||
| } | ||
| } | ||
| } | ||
|
|
||
| actual class MultiChoiceSegmentedButtonRowScope(val elementScope: ElementScope<HTMLElement>) { | ||
| @MaterialWebLabsApi | ||
| @Composable | ||
| actual fun SegmentedButton( | ||
| selected: Boolean, | ||
| onClick: () -> Unit, | ||
| modifier: Modifier, | ||
| enabled: Boolean, | ||
| icon: @Composable (() -> Unit)?, | ||
| label: @Composable () -> Unit | ||
| ) { | ||
| MdOutlinedSegmentedButton( | ||
| disabled = enabled.isFalseOrNull(), | ||
| selected = selected.isTrueOrNull(), | ||
| hasIcon = (icon != null).isTrueOrNull(), | ||
| attrs = modifier.toAttrs { | ||
| this.onClick { onClick() } | ||
| } | ||
| ) { | ||
| icon?.let { iconContent -> | ||
| Div({ | ||
| slotEqIcon() | ||
| }) { | ||
| iconContent() | ||
| } | ||
| } | ||
| label() | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
The TODO comment on line 38 correctly points out that the implementations of SegmentedButton inside SingleChoiceSegmentedButtonRowScope and MultiChoiceSegmentedButtonRowScope are identical. This code duplication should be refactored into a single private function that both scopes can call. This will improve maintainability and reduce the chance of inconsistencies in the future.
| // Note: onValueChangeFinished is not currently supported in the JS implementation | ||
| // TODO: Add support for onValueChangeFinished using appropriate event listener |
There was a problem hiding this comment.
The onValueChangeFinished callback is not supported in the JS implementation of the Slider. This creates a feature disparity between the Compose UI and JS platforms. While noted with a TODO, this is a limitation that developers should be aware of. It would be good to find a way to support this, perhaps by listening to change or pointerup events on the slider element.
No description provided.