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.

Sign up for GitHub

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

Jump to bottom

New ComboBox component #601

Draft
wants to merge 30 commits into
base: main
Choose a base branch
from
Draft

New ComboBox component #601

wants to merge 30 commits into from

Conversation

hamen
Copy link
Collaborator

@hamen hamen commented Sep 18, 2024 •
edited
Loading

Overview

This PR adds the new ComboBox composable. It mirrors Swing ComboBox. AComboBox can be editable or not. A not-editable ComboBox in Swing acts just about like our current Dropdown composable, but the UI is slightly different. Our Dropdown should be replaced by the not-editable ComboBox to enforce consistency between Swing and Compose.

An example of editable ComboBox can be found in IntelliJ Settings → Appearance screen: Zoom and Font size are ComboBoxes. A ComboBox can be made editable using the isEditable parameter.

The UI for editable and not-editable ComboBox is slightly different, as shown in the following screenshots.

Checklist

  • I have executed the Pre-push Gradle task
  • I have performed a self-review of my code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings

How Has This Been Tested?

  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • I have manually tested my changes on MacOS
  • I have manually tested my changes on Windows
  • I have manually tested my changes on Linux
  • I have manually tested my changes with dark theme
  • I have manually tested my changes with light theme

Screenshots

image
image
image
image

Recording2024-09-19153110-ezgif com-video-to-gif-converter
Recording2024-09-19153452-ezgif com-video-to-gif-converter

imageimage

imageimage

@hamen hamen self-assigned this Sep 19, 2024
@hamen hamen added new component A new component to implement consistency Our UI presentation is not consistent with IJ labels Sep 19, 2024
@hamen hamen marked this pull request as ready for review September 19, 2024 13:45
@hamen hamen requested a review from rock3r September 19, 2024 13:45
rock3r
rock3r reviewed Sep 19, 2024
View reviewed changes
Copy link
Collaborator

@rock3r rock3r left a comment •
edited
Loading

Choose a reason for hiding this comment

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

I managed to break the component pretty easily, so I feel there is some work to do still.

First breakage: non-editable ComboBoxes do not remain single line when their content is long.
image

Second breakage, while I was typing in the editable one, I managed to get a crash, not sure how (may be macOS only, given the stack trace):

image

java.lang.reflect.InvocationTargetException
	at java.desktop/sun.lwawt.macosx.LWCToolkit.checkException(LWCToolkit.java:940)
	at java.desktop/sun.lwawt.macosx.LWCToolkit.invokeAndWait(LWCToolkit.java:903)
	at java.desktop/sun.lwawt.macosx.LWCToolkit.invokeAndWait(LWCToolkit.java:834)
	at java.desktop/sun.lwawt.macosx.CInputMethod.invokeAndWaitNoThrow(CInputMethod.java:911)
	at java.desktop/sun.lwawt.macosx.CInputMethod$FxInvoker.invoke(CInputMethod.java:896)
	at java.desktop/sun.lwawt.macosx.CInputMethod.firstRectForCharacterRange(CInputMethod.java:740)
Caused by: java.lang.NullPointerException: Cannot read field "x" because "r" is null
	at java.desktop/sun.lwawt.macosx.CInputMethod.lambda$firstRectForCharacterRange$0(CInputMethod.java:746)
	at java.desktop/sun.lwawt.macosx.CInputMethod$FxInvoker.lambda$invoke$0(CInputMethod.java:899)
	at java.desktop/java.awt.event.InvocationEvent.dispatch(InvocationEvent.java:308)
	at java.desktop/sun.awt.AWTThreading$TrackedInvocationEvent.lambda$dispatch$3(AWTThreading.java:311)
	at java.desktop/sun.awt.AWTThreading$TrackedInvocationEvent.completeIfNotYet(AWTThreading.java:324)
	at java.desktop/sun.awt.AWTThreading$TrackedInvocationEvent.dispatch(AWTThreading.java:311)
	at java.desktop/java.awt.EventQueue.dispatchEventImpl(EventQueue.java:792)
	at java.desktop/java.awt.EventQueue$3.run(EventQueue.java:739)
	at java.desktop/java.awt.EventQueue$3.run(EventQueue.java:733)
	at java.base/java.security.AccessController.doPrivileged(AccessController.java:399)
	at java.base/java.security.ProtectionDomain$JavaSecurityAccessImpl.doIntersectionPrivilege(ProtectionDomain.java:86)
	at java.base/java.security.ProtectionDomain$JavaSecurityAccessImpl.doIntersectionPrivilege(ProtectionDomain.java:97)
	at java.desktop/java.awt.EventQueue$4.run(EventQueue.java:766)
	at java.desktop/java.awt.EventQueue$4.run(EventQueue.java:764)
	at java.base/java.security.AccessController.doPrivileged(AccessController.java:399)
	at java.base/java.security.ProtectionDomain$JavaSecurityAccessImpl.doIntersectionPrivilege(ProtectionDomain.java:86)
	at java.desktop/java.awt.EventQueue.dispatchEvent(EventQueue.java:763)
	at java.desktop/java.awt.EventDispatchThread.pumpOneEventForFilters(EventDispatchThread.java:207)
	at java.desktop/java.awt.EventDispatchThread.pumpEventsForFilter(EventDispatchThread.java:128)
	at java.desktop/java.awt.EventDispatchThread.pumpEventsForHierarchy(EventDispatchThread.java:117)
	at java.desktop/java.awt.EventDispatchThread.pumpEvents(EventDispatchThread.java:113)
	at java.desktop/java.awt.EventDispatchThread.pumpEvents(EventDispatchThread.java:105)
	at java.desktop/java.awt.EventDispatchThread.run(EventDispatchThread.java:92)
java.lang.reflect.InvocationTargetException
	at java.desktop/sun.lwawt.macosx.LWCToolkit.checkException(LWCToolkit.java:940)
	at java.desktop/sun.lwawt.macosx.LWCToolkit.invokeAndWait(LWCToolkit.java:903)
	at java.desktop/sun.lwawt.macosx.LWCToolkit.invokeAndWait(LWCToolkit.java:834)
	at java.desktop/sun.lwawt.macosx.CInputMethod.invokeAndWaitNoThrow(CInputMethod.java:911)
	at java.desktop/sun.lwawt.macosx.CInputMethod$FxInvoker.invoke(CInputMethod.java:896)
	at java.desktop/sun.lwawt.macosx.CInputMethod.firstRectForCharacterRange(CInputMethod.java:740)
Caused by: java.lang.NullPointerException: Cannot read field "x" because "r" is null
	at java.desktop/sun.lwawt.macosx.CInputMethod.lambda$firstRectForCharacterRange$0(CInputMethod.java:746)
	at java.desktop/sun.lwawt.macosx.CInputMethod$FxInvoker.lambda$invoke$0(CInputMethod.java:899)
Caused by: java.lang.NullPointerException: Cannot read field "x" because "r" is null

	at java.desktop/java.awt.event.InvocationEvent.dispatch(InvocationEvent.java:308)
	at java.desktop/sun.awt.AWTThreading$TrackedInvocationEvent.lambda$dispatch$3(AWTThreading.java:311)
	at java.desktop/sun.awt.AWTThreading$TrackedInvocationEvent.completeIfNotYet(AWTThreading.java:324)
	at java.desktop/sun.awt.AWTThreading$TrackedInvocationEvent.dispatch(AWTThreading.java:311)
	at java.desktop/java.awt.EventQueue.dispatchEventImpl(EventQueue.java:792)
	at java.desktop/java.awt.EventQueue$3.run(EventQueue.java:739)
	at java.desktop/java.awt.EventQueue$3.run(EventQueue.java:733)
	at java.base/java.security.AccessController.doPrivileged(AccessController.java:399)
	at java.base/java.security.ProtectionDomain$JavaSecurityAccessImpl.doIntersectionPrivilege(ProtectionDomain.java:86)
	at java.base/java.security.ProtectionDomain$JavaSecurityAccessImpl.doIntersectionPrivilege(ProtectionDomain.java:97)
	at java.desktop/java.awt.EventQueue$4.run(EventQueue.java:766)
	at java.desktop/java.awt.EventQueue$4.run(EventQueue.java:764)
	at java.base/java.security.AccessController.doPrivileged(AccessController.java:399)
	at java.base/java.security.ProtectionDomain$JavaSecurityAccessImpl.doIntersectionPrivilege(ProtectionDomain.java:86)
	at java.desktop/java.awt.EventQueue.dispatchEvent(EventQueue.java:763)
	at java.desktop/java.awt.EventDispatchThread.pumpOneEventForFilters(EventDispatchThread.java:207)
	at java.desktop/java.awt.EventDispatchThread.pumpEventsForFilter(EventDispatchThread.java:128)
	at java.desktop/java.awt.EventDispatchThread.pumpEventsForHierarchy(EventDispatchThread.java:117)
	at java.desktop/java.awt.EventDispatchThread.pumpEvents(EventDispatchThread.java:113)
	at java.desktop/java.awt.EventDispatchThread.pumpEvents(EventDispatchThread.java:105)
	at java.desktop/java.awt.EventDispatchThread.run(EventDispatchThread.java:92)
java.lang.reflect.InvocationTargetException
	at java.desktop/sun.lwawt.macosx.LWCToolkit.checkException(LWCToolkit.java:940)
	at java.desktop/sun.lwawt.macosx.LWCToolkit.invokeAndWait(LWCToolkit.java:903)
	at java.desktop/sun.lwawt.macosx.LWCToolkit.invokeAndWait(LWCToolkit.java:834)
	at java.desktop/sun.lwawt.macosx.CInputMethod.invokeAndWaitNoThrow(CInputMethod.java:911)
	at java.desktop/sun.lwawt.macosx.CInputMethod$FxInvoker.invoke(CInputMethod.java:896)
	at java.desktop/sun.lwawt.macosx.CInputMethod.firstRectForCharacterRange(CInputMethod.java:740)
Caused by: java.lang.NullPointerException: Cannot read field "x" because "r" is null
	at java.desktop/sun.lwawt.macosx.CInputMethod.lambda$firstRectForCharacterRange$0(CInputMethod.java:746)
	at java.desktop/sun.lwawt.macosx.CInputMethod$FxInvoker.lambda$invoke$0(CInputMethod.java:899)
	at java.desktop/java.awt.event.InvocationEvent.dispatch(InvocationEvent.java:308)
	at java.desktop/sun.awt.AWTThreading$TrackedInvocationEvent.lambda$dispatch$3(AWTThreading.java:311)
	at java.desktop/sun.awt.AWTThreading$TrackedInvocationEvent.completeIfNotYet(AWTThreading.java:324)
	at java.desktop/sun.awt.AWTThreading$TrackedInvocationEvent.dispatch(AWTThreading.java:311)
	at java.desktop/java.awt.EventQueue.dispatchEventImpl(EventQueue.java:792)
	at java.desktop/java.awt.EventQueue$3.run(EventQueue.java:739)
	at java.desktop/java.awt.EventQueue$3.run(EventQueue.java:733)
	at java.base/java.security.AccessController.doPrivileged(AccessController.java:399)
	at java.base/java.security.ProtectionDomain$JavaSecurityAccessImpl.doIntersectionPrivilege(ProtectionDomain.java:86)
	at java.base/java.security.ProtectionDomain$JavaSecurityAccessImpl.doIntersectionPrivilege(ProtectionDomain.java:97)
	at java.desktop/java.awt.EventQueue$4.run(EventQueue.java:766)
	at java.desktop/java.awt.EventQueue$4.run(EventQueue.java:764)
	at java.base/java.security.AccessController.doPrivileged(AccessController.java:399)
	at java.base/java.security.ProtectionDomain$JavaSecurityAccessImpl.doIntersectionPrivilege(ProtectionDomain.java:86)
	at java.desktop/java.awt.EventQueue.dispatchEvent(EventQueue.java:763)
	at java.desktop/java.awt.EventDispatchThread.pumpOneEventForFilters(EventDispatchThread.java:207)
	at java.desktop/java.awt.EventDispatchThread.pumpEventsForFilter(EventDispatchThread.java:128)
	at java.desktop/java.awt.EventDispatchThread.pumpEventsForHierarchy(EventDispatchThread.java:117)
	at java.desktop/java.awt.EventDispatchThread.pumpEvents(EventDispatchThread.java:113)
	at java.desktop/java.awt.EventDispatchThread.pumpEvents(EventDispatchThread.java:105)
	at java.desktop/java.awt.EventDispatchThread.run(EventDispatchThread.java:92)
java.lang.reflect.InvocationTargetException
	at java.desktop/sun.lwawt.macosx.LWCToolkit.checkException(LWCToolkit.java:940)
	at java.desktop/sun.lwawt.macosx.LWCToolkit.invokeAndWait(LWCToolkit.java:903)
	at java.desktop/sun.lwawt.macosx.LWCToolkit.invokeAndWait(LWCToolkit.java:834)
	at java.desktop/sun.lwawt.macosx.CInputMethod.invokeAndWaitNoThrow(CInputMethod.java:911)
	at java.desktop/sun.lwawt.macosx.CInputMethod$FxInvoker.invoke(CInputMethod.java:896)
	at java.desktop/sun.lwawt.macosx.CInputMethod.firstRectForCharacterRange(CInputMethod.java:740)
Caused by: java.lang.NullPointerException: Cannot read field "x" because "r" is null
	at java.desktop/sun.lwawt.macosx.CInputMethod.lambda$firstRectForCharacterRange$0(CInputMethod.java:746)
	at java.desktop/sun.lwawt.macosx.CInputMethod$FxInvoker.lambda$invoke$0(CInputMethod.java:899)
	at java.desktop/java.awt.event.InvocationEvent.dispatch(InvocationEvent.java:308)
	at java.desktop/sun.awt.AWTThreading$TrackedInvocationEvent.lambda$dispatch$3(AWTThreading.java:311)
	at java.desktop/sun.awt.AWTThreading$TrackedInvocationEvent.completeIfNotYet(AWTThreading.java:324)
	at java.desktop/sun.awt.AWTThreading$TrackedInvocationEvent.dispatch(AWTThreading.java:311)
	at java.desktop/java.awt.EventQueue.dispatchEventImpl(EventQueue.java:792)
	at java.desktop/java.awt.EventQueue$3.run(EventQueue.java:739)
	at java.desktop/java.awt.EventQueue$3.run(EventQueue.java:733)
	at java.base/java.security.AccessController.doPrivileged(AccessController.java:399)
	at java.base/java.security.ProtectionDomain$JavaSecurityAccessImpl.doIntersectionPrivilege(ProtectionDomain.java:86)
	at java.base/java.security.ProtectionDomain$JavaSecurityAccessImpl.doIntersectionPrivilege(ProtectionDomain.java:97)
	at java.desktop/java.awt.EventQueue$4.run(EventQueue.java:766)
	at java.desktop/java.awt.EventQueue$4.run(EventQueue.java:764)
	at java.base/java.security.AccessController.doPrivileged(AccessController.java:399)
	at java.base/java.security.ProtectionDomain$JavaSecurityAccessImpl.doIntersectionPrivilege(ProtectionDomain.java:86)
	at java.desktop/java.awt.EventQueue.dispatchEvent(EventQueue.java:763)
	at java.desktop/java.awt.EventDispatchThread.pumpOneEventForFilters(EventDispatchThread.java:207)
	at java.desktop/java.awt.EventDispatchThread.pumpEventsForFilter(EventDispatchThread.java:128)
	at java.desktop/java.awt.EventDispatchThread.pumpEventsForHierarchy(EventDispatchThread.java:117)
	at java.desktop/java.awt.EventDispatchThread.pumpEvents(EventDispatchThread.java:113)
	at java.desktop/java.awt.EventDispatchThread.pumpEvents(EventDispatchThread.java:105)
	at java.desktop/java.awt.EventDispatchThread.run(EventDispatchThread.java:92)

PS: I don't think the component should get wider as the content gets longer (see first screenshot, the Warning one should not have grown and taken over all the space)

ide-laf-bridge/src/main/kotlin/org/jetbrains/jewel/bridge/theme/IntUiBridge.kt
@@ -489,7 +489,7 @@ private fun readDividerStyle() =
DividerStyle(color = JBColor.border().toComposeColorOrUnspecified(), metrics = DividerMetrics.defaults())

private fun readDefaultDropdownStyle(menuStyle: MenuStyle): DropdownStyle {
val normalBackground = retrieveColorOrUnspecified("ComboBox.nonEditableBackground")
val normalBackground = retrieveColorOrUnspecified("ComboBox.background")
Copy link
Collaborator

Choose a reason for hiding this comment

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

How come we changed this? Was it resolving to the wrong colour?

Copy link
Collaborator

@rock3r rock3r Sep 19, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

image

I think this is wrong, it was correct before. Editable and non-editable have different background colours.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Nice catch. I iterated on colors more, expanding on the existing insufficient DropdownColors.
I have also added more screenshots to the PR description.
Reference commit is 25e5f1a.

.../ide-plugin/src/main/kotlin/org/jetbrains/jewel/samples/ideplugin/SwingComparisonTabPanel.kt Outdated
Comment on lines 204 to 231
val zoomLevels = arrayOf("100%", "125%", "150%", "175%", "200%", "300%")

JPanel()
.apply {
layout = BoxLayout(this, BoxLayout.Y_AXIS)
add(JLabel("Not editable").apply { alignmentX = Component.LEFT_ALIGNMENT })
add(
ComboBox(DefaultComboBoxModel(zoomLevels)).apply {
isEditable = false
alignmentX = Component.LEFT_ALIGNMENT
}
)
}
.run { cell(this).align(AlignY.TOP) }

val itemsComboBox = arrayOf("Cat", "Elephant", "Sun", "Book", "Laughter")
JPanel()
.apply {
layout = BoxLayout(this, BoxLayout.Y_AXIS)
add(JLabel("Editable").apply { alignmentX = Component.LEFT_ALIGNMENT })
add(
ComboBox(DefaultComboBoxModel(itemsComboBox)).apply {
isEditable = true
alignmentX = Component.LEFT_ALIGNMENT
}
)
}
.run { cell(this).align(AlignY.TOP) }
Copy link
Collaborator

Choose a reason for hiding this comment

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

You should use the Kotlin UI DSL for this, not just dump a bunch of JPanels in it :) The label() and comboBox() APIs are available.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'll try again later. That bunch of JPanels works. The DSL was wasting my time.

ui/src/main/kotlin/org/jetbrains/jewel/ui/component/ComboBox.kt Outdated
modifier
.clickable(
onClick = {
// TODO: Trick to skip click event when close menu by click dropdown
Copy link
Collaborator

Choose a reason for hiding this comment

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

Does this still need to be addressed? If not, it should not be a TODO

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It's something we inherited from the Dropdown. It wires the Popup and the clickable area of the ComboBox, covering some focus/click edge-case. I'll remove the TODO because there is nothing to do here, keeping the code.

ui/src/main/kotlin/org/jetbrains/jewel/ui/component/ComboBox.kt Outdated
indication = null,
)
.background(colors.backgroundFor(comboBoxState).value, shape)
.thenIf(outline == Outline.None) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

You have hasNoOutline, you should use it — however:

  1. You should join identical, consecutive thenIf calls (you can chain modifiers in there!)
  2. I am not sure why this is needed, the outline modifier below will draw on top of this anyway (but maybe this results in a slightly cleaner draw output)
  3. I am not sure why the next thenIf check is needed, either, as that can just be moved above the focus outline, and the focus outline should be drawing on top of the border at that point

ui/src/main/kotlin/org/jetbrains/jewel/ui/component/ComboBox.kt
outlineShape = shape,
alignment = Stroke.Alignment.Center,
)
.width(IntrinsicSize.Max)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this needed?

ui/src/main/kotlin/org/jetbrains/jewel/ui/component/ComboBox.kt Outdated
comboBoxState = comboBoxState.copy(focused = it.isFocused)
},
contentAlignment = Alignment.CenterStart,
content = {
Copy link
Collaborator

Choose a reason for hiding this comment

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

You can move this lambda outside the parentheses

ui/src/main/kotlin/org/jetbrains/jewel/ui/component/ComboBox.kt
contentAlignment = Alignment.CenterStart,
) {
CompositionLocalProvider(LocalContentColor provides colors.contentFor(comboBoxState).value) {
Box(
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't follow why the content is not just a Row that contains the content, optionally the vertical line, and the arrow. The box-based approach seems a bit finicky and you're trying to implement a Row-like behaviour anyway

ui/src/main/kotlin/org/jetbrains/jewel/ui/component/ComboBox.kt
thickness = metrics.borderWidth,
color = colors.border,
modifier =
Modifier.align(Alignment.CenterStart).thenIf(comboBoxState.isFocused) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

The fact that you need an extra padding if the component is focused smells a bit to me. Why does the interior "occupiable" size of the component change with focus? That should not happen, as it could cause things jumping around.

ui/src/main/kotlin/org/jetbrains/jewel/ui/component/ComboBox.kt
textStyle: TextStyle = JewelTheme.defaultTextStyle,
menuContent: MenuScope.() -> Unit,
) {
var skipNextClick by remember { mutableStateOf(false) }
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we still need this hack?

hamen and others added 23 commits September 20, 2024 10:20
@hamen
integrate ComboBox in Standalone
91de4d8 
Signed-off-by: Ivan Morgillo <imorgillo@gmail.com>
@hamen
add ComboBoxes to SwingComparisonTabPanel
075e502 
Signed-off-by: Ivan Morgillo <imorgillo@gmail.com>
@hamen
fix text color in ComboBox
beec15e 
Signed-off-by: Ivan Morgillo <imorgillo@gmail.com>
@hamen
add a bit of space below combobox in IDE example
c437de0 
Signed-off-by: Ivan Morgillo <imorgillo@gmail.com>
@hamen
tune ComboBox popup padding
8c5a5d5 
Signed-off-by: Ivan Morgillo <imorgillo@gmail.com>
@hamen
fix ComboBox field background color
24c92ba 
Signed-off-by: Ivan Morgillo <imorgillo@gmail.com>
@hamen
extract textStyle in ComboBox
9b16fcd 
Signed-off-by: Ivan Morgillo <imorgillo@gmail.com>
@hamen
update API files
36a2c88 
Signed-off-by: Ivan Morgillo <imorgillo@gmail.com>
@hamen
make the formatter happy
87c0c5d 
Signed-off-by: Ivan Morgillo <imorgillo@gmail.com>
@hamen
fix ComboBox border
2a77f8a 
Signed-off-by: Ivan Morgillo <imorgillo@gmail.com>
@hamen
tune ComboBox focus
6d26e57 
Signed-off-by: Ivan Morgillo <imorgillo@gmail.com>
@hamen
Revert "tune ComboBox focus"
9cd1321 
This reverts commit f336503.
@hamen
iterate on focus
0ba6a94 
Signed-off-by: Ivan Morgillo <imorgillo@gmail.com>
@hamen
reformat
dfbfb90 
Signed-off-by: Ivan Morgillo <imorgillo@gmail.com>
@hamen
remove cursorBrush style
487ae9b 
Signed-off-by: Ivan Morgillo <imorgillo@gmail.com>
@hamen
add isEditable flag to ComboBox
3f5d8bd 
Signed-off-by: Ivan Morgillo <imorgillo@gmail.com>
@hamen
make the formatter happy
43cdcab
@hamen
tune ComboBox menu horizontal padding
0201ab7 
Signed-off-by: Ivan Morgillo <imorgillo@gmail.com>
@hamen
remove offset from Standalone style
4326153 
Signed-off-by: Ivan Morgillo <imorgillo@gmail.com>
@hamen
fix a couple of paddings in the menu
d3f74cd 
Signed-off-by: Ivan Morgillo <imorgillo@gmail.com>
@hamen
make linter and formatter happy
9faca52 
Signed-off-by: Ivan Morgillo <imorgillo@gmail.com>
@hamen
make the formatter happy
d25b1f8
@hamen
add not-editable ComboBox to Standalone
c961caa 
Signed-off-by: Ivan Morgillo <imorgillo@gmail.com>
@hamen
fix focus on not-editable ComboBox
2b65994 
Signed-off-by: Ivan Morgillo <imorgillo@gmail.com>
@hamen
rearrange ComboBox params to make ktlint happy
fe7d89e 
Signed-off-by: Ivan Morgillo <imorgillo@gmail.com>
@hamen
update API files
d573cd5 
Signed-off-by: Ivan Morgillo <imorgillo@gmail.com>
@hamen
Copy link
Collaborator Author

hamen commented Sep 20, 2024

First breakage: non-editable ComboBoxes do not remain single line when their content is long.

Nice catch. In Swing is a bit harder to reproduce in the sample because we use a static list there. In Compose I used a single TextFieldState for all the fields because I wanted to be fancy, and it backfired you found a bug 👍

Anyway, Swing uses a middle ellipsis:
image

We don't have it in Compose right now, but it's coming in 1.8.0, it seems: https://issuetracker.google.com/issues/185418980
I went for the basic end ellipsis for now, and maybe we open a ticket to iterate:
image
I also enforced the single line, that it was already there on the editable version, but I missed it on the not-editable.

Same behavior on Standalone:
image

@hamen
fix single line on not-editable ComboBox
6095e94 
reference: #601 (review)
Signed-off-by: Ivan Morgillo <imorgillo@gmail.com>
@hamen
prevent the ComboBox from expanding
7569b5c 
reference: #601 (review)
Signed-off-by: Ivan Morgillo <imorgillo@gmail.com>
@hamen
Copy link
Collaborator Author

hamen commented Sep 20, 2024

PS: I don't think the component should get wider as the content gets longer (see first screenshot, the Warning one should not have grown and taken over all the space)

It doesn't expand if you provide a Modifier.width(), but, maybe it's not clear enough. For the sake of it, I have introduced a way that prevents the ComboBox from expanding even if you don't specify the width.
It's in 7569b5c.

@hamen hamen marked this pull request as draft September 20, 2024 11:08
@hamen
iterate on colors
25e5f1a 
reference: #601 (comment)
Signed-off-by: Ivan Morgillo <imorgillo@gmail.com>
@hamen
address a couple of feedbacks
029e0e6 
reference: #601 (comment)
Signed-off-by: Ivan Morgillo <imorgillo@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rock3r rock3r rock3r left review comments

At least 1 approving review is required to merge this pull request.

Assignees

@hamen hamen

Labels
consistency Our UI presentation is not consistent with IJ new component A new component to implement
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@hamen @rock3r