malliaridis commented on code in PR #3521:
URL: https://github.com/apache/solr/pull/3521#discussion_r2321979502
##########
solr/ui/src/commonMain/kotlin/org/apache/solr/ui/views/configsets/ConfigsetsDropdown.kt:
##########
@@ -0,0 +1,64 @@
+package org.apache.solr.ui.views.configsets
+
+import androidx.compose.foundation.layout.Row
+import androidx.compose.foundation.layout.fillMaxWidth
+import androidx.compose.foundation.layout.padding
+import androidx.compose.foundation.layout.widthIn
+import androidx.compose.material3.DropdownMenuItem
+import androidx.compose.material3.ExperimentalMaterial3Api
+import androidx.compose.material3.ExposedDropdownMenuBox
+import androidx.compose.material3.ExposedDropdownMenuDefaults
+import androidx.compose.material3.OutlinedTextField
+import androidx.compose.material3.Text
+import androidx.compose.runtime.Composable
+import androidx.compose.runtime.getValue
+import androidx.compose.runtime.mutableStateOf
+import androidx.compose.runtime.remember
+import androidx.compose.runtime.setValue
+import androidx.compose.ui.Alignment
+import androidx.compose.ui.Modifier
+import androidx.compose.ui.unit.dp
+import org.apache.solr.ui.components.configsets.data.Configset
+
+@OptIn(ExperimentalMaterial3Api::class)
+@Composable
+fun ConfigsetsDropdown(
+ selectedConfigSet: String,
+ selectConfigset: (String) -> Unit,
+ availableConfigsets: List<Configset>,
+ modifier: Modifier = Modifier,
+) {
+ var expanded by remember { mutableStateOf(false) }
Review Comment:
I would likely want to add this to the component state, so that when the
user clicks on it, we can perform some tasks later, like "silent" refresh in
the background (if that makes sense at some point). So the more is stored in
the component, the more we are in control of user interactions.
But we may also leave it as it is for now, and only update it later.
##########
solr/ui/src/commonMain/kotlin/org/apache/solr/ui/views/configsets/ConfigsetsNavBar.kt:
##########
@@ -43,38 +43,32 @@ import androidx.compose.ui.Alignment
import androidx.compose.ui.Modifier
import androidx.compose.ui.text.style.TextOverflow
import androidx.compose.ui.unit.dp
-import org.apache.solr.ui.components.configsets.data.ListConfigsets
+import org.apache.solr.ui.components.configsets.data.Configset
import org.apache.solr.ui.generated.resources.Res
-import org.apache.solr.ui.generated.resources.configsets_files
import org.apache.solr.ui.generated.resources.configsets_index_query
-import org.apache.solr.ui.generated.resources.configsets_overview
-import org.apache.solr.ui.generated.resources.configsets_request_handlers
-import org.apache.solr.ui.generated.resources.configsets_schema
import org.apache.solr.ui.generated.resources.configsets_search_components
import org.apache.solr.ui.generated.resources.configsets_update_configuration
+import org.apache.solr.ui.generated.resources.files
+import org.apache.solr.ui.generated.resources.overview
+import org.apache.solr.ui.generated.resources.schema
import org.apache.solr.ui.views.navigation.configsets.ConfigsetsTab
import org.jetbrains.compose.resources.stringResource
+
@Composable
-fun ConfigsetsNavBarComponent(
+fun ConfigsetsNavBar(
selectedTab: ConfigsetsTab,
selectTab: (ConfigsetsTab) -> Unit,
- selectedConfigSet: String,
- selectConfigset: (String) -> Unit,
- availableConfigsets: ListConfigsets,
+ availableConfigsets: List<Configset>,
modifier: Modifier = Modifier,
- content: @Composable (tab: ConfigsetsTab, configset: String) -> Unit = {
tab, _ ->
- Box(Modifier.fillMaxSize(), contentAlignment = Alignment.Center) {
Text(stringResource(tabLabelRes(tab))) }
- },
) {
- if (availableConfigsets.names.isEmpty()) {
+ if (availableConfigsets.isEmpty()) {
Box(Modifier.fillMaxSize(), contentAlignment = Alignment.Center) {
Text("No configsets available")
}
return
}
Review Comment:
I would normally move this logic to the parent (`ConfigsetsContent`), so
that the NavBar does not have any reference to information it doesnt use.
Render the `ConfigsetsNavBar` only if the availableConfigsets is not empty (in
parent), otherwise render this box. And you may still add to the
`ConfigsetComponent.Model` a state value like `hasConfigsets: boolean` that you
can directly use (and populate during mapping of state to model), so that you
don't need to run a function on a collection in the UI (even if it is a cheap
operation here).
Most important though is that I would not hide the navigation elements
(UI/UX). Navigation elements in general should stay where they are to maintain
recognizability (users should see the screen and directly know where they are
by the static elements like navigation options). They should also know what
they can do here, even for a fresh installation. Most you could do (and I would
not even do that) is to disable the navigation options. But instead, I would
support in each section the case where no configset is selected yet, so that
user can still navigate through the tabs and see what the content is, before
actually taking any actions or loading any content.
When no configset is selected, the tab may display the sections with small
hints (like "Here you can manage the configsets files. Select a configset to
get started." for the Files tab). But that is out of scope of this PR.
##########
solr/ui/src/commonTest/kotlin/org/apache/solr/ui/views/configsets/ConfigsetsContentTest.kt:
##########
@@ -0,0 +1,128 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.solr.ui.views.configsets
+
+import androidx.compose.ui.test.ExperimentalTestApi
+import androidx.compose.ui.test.onNodeWithText
+import androidx.compose.ui.test.performClick
+import androidx.compose.ui.test.runComposeUiTest
+import com.arkivanov.decompose.router.stack.ChildStack
+import com.arkivanov.decompose.value.MutableValue
+import com.arkivanov.decompose.value.Value
+import kotlin.test.Test
+import kotlin.test.assertEquals
+import kotlin.test.assertFalse
+import kotlinx.coroutines.flow.MutableStateFlow
+import kotlinx.coroutines.flow.StateFlow
+import org.apache.solr.ui.components.configsets.ConfigsetsComponent
+import org.apache.solr.ui.components.configsets.data.Configset
+import
org.apache.solr.ui.components.configsets.overview.ConfigsetsOverviewComponent
+import org.apache.solr.ui.views.navigation.configsets.ConfigsetsTab
+
+@OptIn(ExperimentalTestApi::class)
+class ConfigsetsContentTest {
+
+ @Test
+ fun emptyList_showsPlaceholder() = runComposeUiTest {
+ val comp = TestConfigsetsComponent(emptyList())
+
+ setContent { ConfigsetsContent(component = comp) }
+
+ // Placeholder text from the TextField
+ onNodeWithText("No configsets available").assertExists()
+
+ // Field click shouldn’t open the menu (anchor is disabled in our impl)
+ onNodeWithText("No configsets available").performClick()
+ runOnIdle { assertFalse(comp.model.value.expanded) }
+ }
+
+ @Test
+ fun selectItem_updatesSelection_andClosesMenu() = runComposeUiTest {
+ val comp = TestConfigsetsComponent(
+ names = listOf("gettingstarted", "techproducts"),
+ selected = "gettingstarted",
+ )
+
+ setContent { ConfigsetsContent(component = comp) }
+
+ // Open
+ onNodeWithText("gettingstarted").assertExists().performClick()
+
+ // Select another
+ onNodeWithText("techproducts").assertExists().performClick()
+
+ // State updated + menu closed
+ runOnIdle {
+ assertEquals("techproducts", comp.model.value.selectedConfigset)
+ assertFalse(comp.model.value.expanded)
+ }
+ }
+
+ @Test
+ fun testSubsectionSwitch() = runComposeUiTest {
+ val comp = TestConfigsetsComponent(names = listOf("basic_configs"))
+
+ setContent { ConfigsetsContent(component = comp) }
+
+ // Click using the text node
+ onNodeWithText("Schema").assertExists().performClick()
+
+ runOnIdle { assertEquals(ConfigsetsTab.Schema,
comp.model.value.selectedTab) }
+ }
+}
+
+private object DummyOverviewComponent : ConfigsetsOverviewComponent
+
+class TestConfigsetsComponent(
+ names: List<String>,
+ selected: String = "",
+ expanded: Boolean = false,
+) : ConfigsetsComponent {
+
+ private val _model = MutableStateFlow(
+ ConfigsetsComponent.Model(
+ selectedTab = ConfigsetsTab.Overview,
+ configsets = names.map(::Configset),
+ selectedConfigset = selected,
+ expanded = expanded,
+ ),
+ )
+ override val model: StateFlow<ConfigsetsComponent.Model> = _model
Review Comment:
You may simplify your test component and your access in tests to your model
values by not using a mutable state flow, but rather static fields where you
can directly access. See for example
[TestBasicAuthComponent](https://github.com/apache/solr/blob/main/solr/ui/src/commonTest/kotlin/org/apache/solr/ui/views/auth/TestBasicAuthComponent.kt).
Of course the way you did it isn't wrong or bad, it is just less readable when
accessing a field with `comp.model.value.selectedTab` instead of just
`comp.selectedTab`. And you are close to start writing components that you have
to test again due to the logic behind setting a field value and the
relationship between fields. Imagine the case where you have `isValidEmail:
Boolean` and `email: String` as state in your component. What would be the
logic in your test component?
##########
solr/ui/src/commonTest/kotlin/org/apache/solr/ui/views/configsets/ConfigsetsContentTest.kt:
##########
@@ -0,0 +1,128 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.solr.ui.views.configsets
+
+import androidx.compose.ui.test.ExperimentalTestApi
+import androidx.compose.ui.test.onNodeWithText
+import androidx.compose.ui.test.performClick
+import androidx.compose.ui.test.runComposeUiTest
+import com.arkivanov.decompose.router.stack.ChildStack
+import com.arkivanov.decompose.value.MutableValue
+import com.arkivanov.decompose.value.Value
+import kotlin.test.Test
+import kotlin.test.assertEquals
+import kotlin.test.assertFalse
+import kotlinx.coroutines.flow.MutableStateFlow
+import kotlinx.coroutines.flow.StateFlow
+import org.apache.solr.ui.components.configsets.ConfigsetsComponent
+import org.apache.solr.ui.components.configsets.data.Configset
+import
org.apache.solr.ui.components.configsets.overview.ConfigsetsOverviewComponent
+import org.apache.solr.ui.views.navigation.configsets.ConfigsetsTab
+
+@OptIn(ExperimentalTestApi::class)
+class ConfigsetsContentTest {
+
+ @Test
+ fun emptyList_showsPlaceholder() = runComposeUiTest {
+ val comp = TestConfigsetsComponent(emptyList())
+
+ setContent { ConfigsetsContent(component = comp) }
+
+ // Placeholder text from the TextField
+ onNodeWithText("No configsets available").assertExists()
+
+ // Field click shouldn’t open the menu (anchor is disabled in our impl)
+ onNodeWithText("No configsets available").performClick()
+ runOnIdle { assertFalse(comp.model.value.expanded) }
Review Comment:
To keep it simple with less nesting, prefer `advanceUntilIdle()`
##########
solr/ui/src/commonMain/kotlin/org/apache/solr/ui/views/configsets/ConfigsetsDropdown.kt:
##########
@@ -0,0 +1,64 @@
+package org.apache.solr.ui.views.configsets
+
+import androidx.compose.foundation.layout.Row
+import androidx.compose.foundation.layout.fillMaxWidth
+import androidx.compose.foundation.layout.padding
+import androidx.compose.foundation.layout.widthIn
+import androidx.compose.material3.DropdownMenuItem
+import androidx.compose.material3.ExperimentalMaterial3Api
+import androidx.compose.material3.ExposedDropdownMenuBox
+import androidx.compose.material3.ExposedDropdownMenuDefaults
+import androidx.compose.material3.OutlinedTextField
+import androidx.compose.material3.Text
+import androidx.compose.runtime.Composable
+import androidx.compose.runtime.getValue
+import androidx.compose.runtime.mutableStateOf
+import androidx.compose.runtime.remember
+import androidx.compose.runtime.setValue
+import androidx.compose.ui.Alignment
+import androidx.compose.ui.Modifier
+import androidx.compose.ui.unit.dp
+import org.apache.solr.ui.components.configsets.data.Configset
+
+@OptIn(ExperimentalMaterial3Api::class)
+@Composable
+fun ConfigsetsDropdown(
+ selectedConfigSet: String,
+ selectConfigset: (String) -> Unit,
+ availableConfigsets: List<Configset>,
+ modifier: Modifier = Modifier,
+) {
+ var expanded by remember { mutableStateOf(false) }
+ Row(
+ modifier = Modifier
+ .fillMaxWidth()
+ .padding(horizontal = 16.dp, vertical = 8.dp),
+ verticalAlignment = Alignment.CenterVertically,
+ ) {
+ ExposedDropdownMenuBox(
+ expanded = expanded,
+ onExpandedChange = { expanded = !expanded },
+ modifier = Modifier.widthIn(min = 256.dp).weight(1f),
+ ) {
+ OutlinedTextField(
+ value = selectedConfigSet,
+ onValueChange = {},
+ readOnly = true,
+ label = { Text("Configset") },
Review Comment:
I believe there are still some values that can be localized?
##########
solr/ui/src/commonTest/kotlin/org/apache/solr/ui/views/configsets/ConfigsetsContentTest.kt:
##########
@@ -0,0 +1,128 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.solr.ui.views.configsets
+
+import androidx.compose.ui.test.ExperimentalTestApi
+import androidx.compose.ui.test.onNodeWithText
+import androidx.compose.ui.test.performClick
+import androidx.compose.ui.test.runComposeUiTest
+import com.arkivanov.decompose.router.stack.ChildStack
+import com.arkivanov.decompose.value.MutableValue
+import com.arkivanov.decompose.value.Value
+import kotlin.test.Test
+import kotlin.test.assertEquals
+import kotlin.test.assertFalse
+import kotlinx.coroutines.flow.MutableStateFlow
+import kotlinx.coroutines.flow.StateFlow
+import org.apache.solr.ui.components.configsets.ConfigsetsComponent
+import org.apache.solr.ui.components.configsets.data.Configset
+import
org.apache.solr.ui.components.configsets.overview.ConfigsetsOverviewComponent
+import org.apache.solr.ui.views.navigation.configsets.ConfigsetsTab
+
+@OptIn(ExperimentalTestApi::class)
+class ConfigsetsContentTest {
+
+ @Test
+ fun emptyList_showsPlaceholder() = runComposeUiTest {
+ val comp = TestConfigsetsComponent(emptyList())
+
+ setContent { ConfigsetsContent(component = comp) }
+
+ // Placeholder text from the TextField
+ onNodeWithText("No configsets available").assertExists()
+
+ // Field click shouldn’t open the menu (anchor is disabled in our impl)
+ onNodeWithText("No configsets available").performClick()
+ runOnIdle { assertFalse(comp.model.value.expanded) }
+ }
+
+ @Test
+ fun selectItem_updatesSelection_andClosesMenu() = runComposeUiTest {
Review Comment:
You are mixing here UI test with integration test of your test component
(instead of your actual component). It is hard to explain this, but let me try
to break it down anyway.
The statement "select item updates selection and closes menu" is a test that
tests the behavior of your test component, and not testing the UI itself. Pure
UI tests (that use the test component) would be:
- WHEN dropdown clicked THEN setMenuExpanded is called
- WHEN dropdown item clicked THEN onSelectConfigset called
Possible integration tests (tests that use the actual implementation of
ConfigsetsComponent) would be:
- GIVEN expanded is true AND some configsets WHEN configset selected THEN
dropdown collapses
- GIVEN expanded is true AND some configsets WHEN configset selected THEN
selectedConfigset changes
- GIVEN expanded is false AND some configsets WHEN expand menu THEN expanded
is true
- GIVEN expanded is false AND no configsets WHEN expand menu THEN expanded
is false
Btw. the interface function `setMenuExpanded` could be renamed to
`onExpandMenu` for better readability, both in tests and in code.
I believe with these test examples it is a bit clearer where to draw the
lines between simple UI tests, that test composable functions with test
components, and actual integration tests, that use actual implementations. One
of the root causes why it is easy to confuse these is that we are providing
both business logic and UI in the same package. If we would use different
packages, it would be easier to distinguish them. business logic does not know
UI composables, and UI composables do not know implementation details of
business logic. We may consider splitting the UI module into two modules in the
future (if that makes sense to have two modules for the UI).
##########
solr/ui/src/commonTest/kotlin/org/apache/solr/ui/views/configsets/ConfigsetsContentTest.kt:
##########
@@ -0,0 +1,128 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.solr.ui.views.configsets
+
+import androidx.compose.ui.test.ExperimentalTestApi
+import androidx.compose.ui.test.onNodeWithText
+import androidx.compose.ui.test.performClick
+import androidx.compose.ui.test.runComposeUiTest
+import com.arkivanov.decompose.router.stack.ChildStack
+import com.arkivanov.decompose.value.MutableValue
+import com.arkivanov.decompose.value.Value
+import kotlin.test.Test
+import kotlin.test.assertEquals
+import kotlin.test.assertFalse
+import kotlinx.coroutines.flow.MutableStateFlow
+import kotlinx.coroutines.flow.StateFlow
+import org.apache.solr.ui.components.configsets.ConfigsetsComponent
+import org.apache.solr.ui.components.configsets.data.Configset
+import
org.apache.solr.ui.components.configsets.overview.ConfigsetsOverviewComponent
+import org.apache.solr.ui.views.navigation.configsets.ConfigsetsTab
+
+@OptIn(ExperimentalTestApi::class)
+class ConfigsetsContentTest {
+
+ @Test
+ fun emptyList_showsPlaceholder() = runComposeUiTest {
+ val comp = TestConfigsetsComponent(emptyList())
+
+ setContent { ConfigsetsContent(component = comp) }
+
+ // Placeholder text from the TextField
+ onNodeWithText("No configsets available").assertExists()
Review Comment:
When we later introduce localization, these tests may fail becasue the text
may be localized during tests and not found via `onNodeWithText`. Therefore, it
is recommended to use `Modifier.testTag(...)` in the composables to check if
specific composables are present.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
