gerlowskija commented on code in PR #2605: URL: https://github.com/apache/solr/pull/2605#discussion_r1707007092
########## dev-docs/ui/testing-and-deployment.adoc: ########## @@ -0,0 +1,19 @@ += Testing and Deployment + +== Testing + Review Comment: [0] Not important, just leaving this comment as a reminder that these docs should be populated at some point if this moves beyond POC. ########## dev-docs/ui/component-development.adoc: ########## @@ -0,0 +1,95 @@ += Component Development +:toc: left + +== Overview + +The following list contains a possible approach for implementing a new UI component: + +1. Create new design or start with an existing design, see for example Figma +2. Validate the use case and analyze the components that may be used for the implementation +3. Create Composables that represent the UI component(s) and use placeholders for data population +4. Create a component interface and implementation with the UI state and UI component interactions +5. Create previews with preview component implementations to check the UI implementation +6. Create a store and store provider for fetching resources and interacting with Solr backend +7. Implement the client used by the store provider +8. Write test and test the new component +9. If not already done, integrate component in the existing application +10. If not already done, extract resources like texts to allow internationalization and localization + +It is recommended to take a look at existing components, so that you get a better understanding +on how things are implemented, what they have in common, and how each technology is utilized. + +== Component's Logic + +=== Components (Decompose) + +The component integration interacts with the UI composables and the state store. + +The implementation of the component interface "catches" user inputs like clicks and passes them +to the store as `Intent`s. The intents are then handled by the store implementation and +may send a request to the backend and / or update the store state. The component is consuming +and mapping the store state to the UI state. So once the store state is updated, it will +reflect the changes in the UI. + +=== State Stores and Store Providers + +The state stores manage the state of the application, but independent of the state that is +represented in the UI. Instances are created by store providers that hold the logic of the +store. + +Store providers consist of an executor implementation that consumes actions and intents and +creates messages and labels, a reducer that updates the store state with the messages produced +by the executor, and a function for retrieving an instance of the store. + +The store provider does also define the interface for the client that has to be provided in +order for the executor to make API calls and interact with the Solr backend. + +== Component's Visuals + +=== Composables + +Composables are the UI elements that are defined and styled. They can be seen as boxes, rows and +columns that are nested and change their style and structure based on conditions, state and input. + +There are many ways to get started, but the easiest way probably is to get familiar with the basics +and try things out. The Figma designs make use of almost the same elements for designing, +so the structure and configurations there may be mapped almost one-by-one in Compose code. + +=== Styling + +The styling in Compose is done via `Modifier`s. Each composable should normally accept a modifier +as parameter, so that the user can customize specific visual parameters of the composable like +width, height and alignment in the parent's composable. + +Since we are using Material 3, you do not have to care much about colors, typography and shapes. +These are configured for the entire app, and you only have to make use of the right properties +that are provided by the theme. + +=== Accessibility + +Compose comes with many accessibility features that can be used to improve the user experience. + +The simplest form of accessibility in UI is probably the responsiveness of the UI. this is +realized with `WindowSizeClass`. Some compsables may use a wrapper (usually suffixed with `Content`) Review Comment: [0] wanted to flag the typo in "compsables" in case this PR comes out of draft mode at some point. ########## solr/compose-ui/README.md: ########## @@ -0,0 +1,32 @@ +# Compose Admin UI Review Comment: [+1] Great docs, thank you! ########## solr/compose-ui/src/commonMain/kotlin/org/apache/solr/composeui/components/environment/data/JavaPropertiesResponse.kt: ########## @@ -0,0 +1,32 @@ +/* + * 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.composeui.components.environment.data + +import kotlinx.serialization.SerialName +import kotlinx.serialization.Serializable + +/** + * Response class of the `java-properties` API endpoint. Review Comment: [0] Alright, to check my understanding: 1. This class represents the response of a Solr API, presumably `/api/node/properties` 2. This PR doesn't do any client-generation using the OAS - it creates its own Solr API client, response parsing etc. (That understanding is based largely on files like `HttpEnvironmentStoreClient.kt`) If both of those are correct, my main question is mostly around organization. Is there a reason this response model lives in the 'environment' component? Coming from a non-UI Java world, my naive expectation would be that the UI component code would be separated from the API response-modeling and the Solr-API-client code. Do Compose projects (or UI projects in general) organize things differently? (Or maybe my confusion stems from an overly-narrow understanding of "component" as being essentially synonymous with "UI widget"?) ########## solr/compose-ui/src/commonMain/kotlin/org/apache/solr/composeui/ui/environment/EnvironmentContent.kt: ########## @@ -0,0 +1,123 @@ +/* + * 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.composeui.ui.environment + +import androidx.compose.foundation.layout.Arrangement +import androidx.compose.foundation.layout.Column Review Comment: [Q] This isn't the first time they've cropped up, but there's a bunch here so this is about as good a place to comment on them as any... Was initially surprised to see so many android-related imports here, all from the the Jetpack-Compose project afaict. `technology-overview.adoc` mentioned that ComposeMP builds on the principles of J-C. I took that initially to mean "conceptually", but in hindsight you probably also were saying that ComposeMP uses J-C interfaces and objects as well? Just asking to make sure I understand the relationship between all the moving pieces! ########## solr/compose-ui/src/commonMain/kotlin/org/apache/solr/composeui/ui/theme/Colors.kt: ########## @@ -0,0 +1,766 @@ +/* + * 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.composeui.ui.theme + +import androidx.compose.material3.ColorScheme +import androidx.compose.material3.darkColorScheme +import androidx.compose.material3.lightColorScheme +import androidx.compose.runtime.Composable +import androidx.compose.runtime.Immutable +import androidx.compose.runtime.staticCompositionLocalOf +import androidx.compose.ui.graphics.Color + +/* + * This file holds color values, classes and functions that are used for customizing + * the color scheme of the material theme to match he Solr theme. + * + * Additional colors are also provided as ExtendedColorScheme. This makes it possible + * to use custom color palettes aside the default color scheme that is available. This + * is especially useful when we want to use colors for states of nodes or highlight + * critical errors or warnings. + */ + +val primaryLight = Color(0xFF9C1F00) Review Comment: [Q] Are these values all things you created by hand, or was there something that generated them? Did they come from a template somewhere? ########## dev-docs/ui/technology-overview.adoc: ########## @@ -0,0 +1,83 @@ += Technology Overview Review Comment: [+1] These are great docs, and I love that they really anticipate the complete lack of knowledge the rest of us have when it comes to Kotlin. Hopefully they'll be helpful to folks! The sheer number of libraries and technologies here is a bit intimidating. 😬 But I'm hoping this is just a side effect of seeing all the relevant libraries listed out. I'm sure they're easy to pick up one-by-one in the code with these docs as a handy reference. ########## kotlin-js-store/yarn.lock: ########## @@ -0,0 +1,3092 @@ +# THIS IS AN AUTOGENERATED FILE. DO NOT EDIT THIS FILE DIRECTLY. Review Comment: [0] I'm not too familiar with `yarn`, but I'm going to skip review of this as it appears to be content autogenerated from other files. (If this is something worth paying closer attention to, please correct me!) We'll need to eventually figure out whether the dep-management can't be brought more into line with what the rest of the Solr build uses. See comments on `gradle/libs.versions.toml` to discuss further. ########## build.gradle: ########## @@ -20,6 +20,10 @@ import java.time.format.DateTimeFormatter plugins { id 'base' + alias(libs.plugins.kotlinMultiplatform) apply false Review Comment: [Q] Do you remember the rationale behind these build changes? Just trying to follow how everything works here... ########## gradle/libs.versions.toml: ########## @@ -0,0 +1,65 @@ +[versions] Review Comment: [Q] The rest of our build puts library versions in the `versions.props` and `versions.lock` files in the repo root, which are read and enforced by a gradle plugin (`palantir:consistent-versions`, iirc) to ensure we're not pulling in many versions of the same dep. Is the separate version-management scheme here just a part of the POC, or do you think this is something we'd need to retain in the eventual non-POC version of this PR as well? ########## solr/compose-ui/src/commonMain/kotlin/org/apache/solr/composeui/components/environment/integration/Mappers.kt: ########## Review Comment: [Q] Is there a Kotlin/Compose convention underlying these 'Mapper.kt' files that I'm missing? There's a few of them in this PR, but I'm struggling a bit to understand their purpose. Maybe it'll be clearer as I proceed through the PR... ########## solr/api/build.gradle: ########## @@ -16,8 +18,8 @@ */ plugins { - id 'io.swagger.core.v3.swagger-gradle-plugin' version '2.2.2' - id "org.openapi.generator" version "6.0.1" + alias(libs.plugins.swagger3Core) Review Comment: [Q] Do you remember the rationale behind these build changes? Just trying to follow how everything works here... Some appear to be mostly cosmetic (e.g. the L50 changes below), but I may be missing some subtlety there? ########## solr/compose-ui/src/commonMain/kotlin/org/apache/solr/composeui/components/main/MainComponent.kt: ########## @@ -0,0 +1,91 @@ +/* + * 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.composeui.components.main + +import com.arkivanov.decompose.router.stack.ChildStack +import com.arkivanov.decompose.value.Value +import kotlinx.coroutines.flow.StateFlow +import org.apache.solr.composeui.components.environment.EnvironmentComponent +import org.apache.solr.composeui.components.logging.LoggingComponent +import org.apache.solr.composeui.components.navigation.NavigationComponent +import org.apache.solr.composeui.ui.navigation.MainMenu + +/** + * Main component of the application that is used as base for users with access. + * + * Note that this component can be accessed if the user is either authenticated or if the Solr + * instance accessed does not have any authentication enabled. + */ +interface MainComponent : NavigationComponent { + + /** + * Child stack that holds the navigation state. + */ + val childStack: Value<ChildStack<*, Child>> + + /** + * Handles navigation requests from a navigation menu. + * + * @param menuItem The destination to navigate to. + */ + fun onNavigate(menuItem: MainMenu) + + /** + * Child interface that defines all available children of the [MainComponent]. + */ + sealed interface Child { + + // TODO Uncomment once DashboardComponent available Review Comment: [Q] These are all placeholders for UI screens that aren't currently implemented? ########## solr/compose-ui/src/commonMain/composeResources/font/firacode-variable.ttf: ########## Review Comment: [Q] UI novice question: is it normal/standard in UI projects to store these font files in version control? The existing Solr UI doesn't have any, afaict. Though I'm willing to chalk that up to it being idiosyncratic, outdated, etc. ########## solr/compose-ui/src/commonMain/kotlin/org/apache/solr/composeui/ui/icons/SolrLogo.kt: ########## @@ -0,0 +1,257 @@ +/* + * 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.composeui.ui.icons + +import androidx.compose.foundation.layout.Box +import androidx.compose.material.icons.materialPath +import androidx.compose.material3.Icon +import androidx.compose.material3.MaterialTheme +import androidx.compose.runtime.Composable +import androidx.compose.ui.Modifier +import androidx.compose.ui.graphics.vector.ImageVector +import androidx.compose.ui.graphics.vector.rememberVectorPainter +import androidx.compose.ui.unit.dp +import org.apache.solr.compose_ui.generated.resources.Res +import org.apache.solr.compose_ui.generated.resources.cd_solr_logo +import org.jetbrains.compose.resources.stringResource + +/** + * Colorized Solr logo. + * + * @param modifier Modifier that is applied to the wrapper of the logo. + */ +@Composable +fun SolrLogo( + modifier: Modifier = Modifier, +) { + val solrPainter = rememberVectorPainter( + ImageVector.Builder( + name = "Logos.SolrLogo", + defaultWidth = 128.dp, + defaultHeight = 64.dp, + viewportWidth = 128f, + viewportHeight = 64f, + autoMirror = false, + ).apply { + materialPath { + moveTo(25.7013f, 44.178f) Review Comment: [Q] Wait, is this code drawing a Solr logo from individual points? 🤯 How did you figure all these float coordinate values out, and how long did that take? Man, the work you put into this PR just seems astronomical. The project has png and other logo artifacts that we could use, unless there's some benefit to drawing the logo "by hand" as this code does? ########## solr/compose-ui/build.gradle.kts: ########## @@ -0,0 +1,134 @@ +/* Review Comment: [0] The [gradle docs](https://docs.gradle.org/current/userguide/migrating_from_groovy_to_kotlin_dsl.html) mention that not all IDEs fully support Gradle's Kotlin DSL. Maybe that warning is out of date. But if it's not, we might want to eventually migrate to the Groovy DSL to accommodate project devs who don't use IntelliJ. (I say this reluctantly as someone who hates the Groovy DSL). ########## solr/compose-ui/src/commonMain/kotlin/org/apache/solr/composeui/ui/main/MainContent.kt: ########## @@ -0,0 +1,78 @@ +/* + * 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.composeui.ui.main + +import androidx.compose.foundation.layout.Row +import androidx.compose.foundation.layout.fillMaxHeight +import androidx.compose.foundation.layout.fillMaxWidth +import androidx.compose.foundation.layout.padding +import androidx.compose.foundation.layout.width +import androidx.compose.foundation.rememberScrollState +import androidx.compose.foundation.verticalScroll +import androidx.compose.runtime.Composable +import androidx.compose.runtime.getValue +import androidx.compose.ui.Modifier +import androidx.compose.ui.unit.dp +import com.arkivanov.decompose.extensions.compose.stack.Children +import com.arkivanov.decompose.extensions.compose.subscribeAsState +import org.apache.solr.composeui.components.main.MainComponent +import org.apache.solr.composeui.components.main.integration.asMainMenu +import org.apache.solr.composeui.ui.environment.EnvironmentContent +import org.apache.solr.composeui.ui.logging.LoggingContent +import org.apache.solr.composeui.ui.navigation.NavigationSideBar + +/** + * The composable used for users that have already authenticated. + * + * @param component Component that manages the state of the composable. + */ +@Composable +fun MainContent( + component: MainComponent, + modifier: Modifier = Modifier, +) { + val childStack by component.childStack.subscribeAsState() + val scrollState = rememberScrollState() + + Row(modifier = modifier) { + NavigationSideBar( + modifier = Modifier.fillMaxHeight() + .width(224.dp), + selectedItem = childStack.active.instance.asMainMenu, + onNavigate = component::onNavigate, + ) + + Children( + stack = component.childStack, + modifier = Modifier.weight(1f), + ) { + when(val child = it.instance) { + is MainComponent.Child.Environment -> EnvironmentContent( + component = child.component, + modifier = Modifier.fillMaxWidth() + .verticalScroll(scrollState) + .padding(16.dp), Review Comment: [Q] UI Novice Question: How do you arrive at padding, width, etc. values like the one here? Is it generally guess-and-check using your best judgement on a few form-factors? Or is there a more guided/structured way to do it? ########## solr/compose-ui/src/commonMain/kotlin/org/apache/solr/composeui/ui/navigation/MainMenu.kt: ########## @@ -0,0 +1,38 @@ +/* + * 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.composeui.ui.navigation + +/** Review Comment: [+1] Really appreciate these comments, thank you! ########## solr/compose-ui/src/commonMain/kotlin/org/apache/solr/composeui/utils/HttpClientUtils.kt: ########## @@ -0,0 +1,43 @@ +/* + * 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.composeui.utils + +import io.ktor.client.HttpClient +import io.ktor.client.plugins.contentnegotiation.ContentNegotiation +import io.ktor.client.plugins.defaultRequest +import io.ktor.serialization.kotlinx.json.json +import kotlinx.serialization.json.Json + +/** + * Function that returns a simple HTTP client that is preconfigured with a base + * URL. + */ +fun getDefaultClient() = HttpClient { Review Comment: [Q] I guess this is where authentication logic would need to be hooked in at some point, to ensure that client requests have whatever headers, etc. are necessary for Solr to accept the requests? ########## solr/compose-ui/src/commonMain/kotlin/org/apache/solr/composeui/ui/environment/VersionsCard.kt: ########## @@ -0,0 +1,101 @@ +/* + * 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.composeui.ui.environment + +import androidx.compose.foundation.layout.Arrangement +import androidx.compose.foundation.layout.Row +import androidx.compose.foundation.layout.fillMaxWidth +import androidx.compose.foundation.layout.width +import androidx.compose.material3.MaterialTheme +import androidx.compose.material3.Text +import androidx.compose.runtime.Composable +import androidx.compose.ui.Modifier +import androidx.compose.ui.text.style.TextAlign +import androidx.compose.ui.unit.dp +import org.apache.solr.composeui.components.environment.data.JvmData +import org.apache.solr.composeui.components.environment.data.Versions +import org.apache.solr.composeui.ui.components.SolrCard + +/** + * Composable card that displays system values related to versions. + * + * @param versions Solr versions to display. + * @param jvm JVM values to display. + * @param modifier Modifier to apply to the root composable. + */ +@Composable +internal fun VersionsCard( Review Comment: [Q] UI novice question: Is the term "Card" synonymous with a UI element or widget? Is it any UI element, or is there some particular subset of those that are "cards"? ########## gradle/libs.versions.toml: ########## @@ -0,0 +1,65 @@ +[versions] Review Comment: [0] Not necessary at this point, but leaving a comment here as a reminder that eventually we'll need to license-check all of these deps, if this logic doesn't get integrated with versions.props/versions.lock (see comment above) -- 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: issues-unsubscr...@solr.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org --------------------------------------------------------------------- To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org For additional commands, e-mail: issues-h...@solr.apache.org
