Create a Spatial layout for the Loading portion of CreationScreen #134
Conversation
This change allows for more types of backgrounds and minimum height requirements on the panels.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Summary of Changes
Hello @devbridie, 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 enhances the user experience for XR devices by introducing a specialized spatial layout for the CreationScreen's loading phase. It refactors existing UI components to be more adaptable, allowing the application to dynamically adjust its presentation based on the device's layout type. The changes also improve the flexibility of background rendering, supporting diverse visual requirements for spatial environments.
Highlights
- Spatial Loading Screen for XR: Introduced a dedicated
LoadingScreenSpatiallayout specifically designed for XR devices, providing an optimized visual experience during the loading phase of theCreationScreen. - Generic Background Components: Refactored background handling into new
BackgroundSubspaceandFillBackgroundcomposables. These components are now more generic, supporting configurable aspect ratios, minimum heights, and various drawable resources, allowing for flexible background designs. - Dynamic Loading Screen Layout: The main
LoadingScreencomposable has been refactored to dynamically select its layout based on anEditScreenLayoutTypeparameter. It now intelligently switches between the new spatial layout and traditional scaffold layouts (compact/medium) as appropriate. - Resource Management and New Background: The
squiggle_full.xmldrawable resource was moved from thecore/thememodule tocore/xr, and a newsquiggle_light.xmldrawable was added to provide a distinct background for the spatial loading screen.
Using Gemini Code Assist
The 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 in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
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 .gemini/ folder in the base of the repository. Detailed instructions can be found here.
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
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
There was a problem hiding this comment.
Code Review
This pull request introduces a new spatial layout for the loading screen on XR devices, which is a great addition. The refactoring of background components into more generic and reusable composables like BackgroundSubspace and FillBackground is well-executed. The LoadingScreen has also been nicely updated to handle different layout types.
My feedback focuses on improving code clarity and maintainability by addressing hardcoded values ('magic numbers') and suggesting a more idiomatic Compose API design for LoadingScreenScaffold. These changes will make the code easier to read and manage in the future.
| SpatialBoxScope.() -> Unit, | ||
| ) { | ||
| BackgroundSubspace( | ||
| aspectRatio = 1.7f, |
There was a problem hiding this comment.
The aspect ratio 1.7f is a hardcoded value. Consider extracting it into a named constant to improve readability and make it easier to manage, especially if this value is used elsewhere or might change.
| FillBackground(drawable) | ||
| Subspace { | ||
| SpatialBox(SubspaceModifier.offset(z = 10.dp), content = content) | ||
| SpatialBox(SubspaceModifier.offset(z = 10.dp).fillMaxSize(), content = content) |
There was a problem hiding this comment.
The z-offset 10.dp is a magic number. It's good practice to define such values as named constants (e.g., private val CONTENT_Z_OFFSET = 10.dp) to clarify their purpose (e.g., preventing z-fighting) and make them easier to find and update.
| ) { contentPadding -> | ||
| LoadingScreenContents(contentPadding) | ||
| Box( | ||
| modifier = Modifier | ||
| .fillMaxSize() | ||
| .padding(contentPadding), | ||
| ) { | ||
| content() | ||
| } | ||
| } |
There was a problem hiding this comment.
The current implementation of LoadingScreenScaffold applies padding inside a Box, which is less flexible than the standard Scaffold pattern. A more idiomatic approach is to pass the PaddingValues to the content lambda, giving the caller more control over how padding is applied. This would require changing the content lambda's signature to @Composable (padding: PaddingValues) -> Unit and updating the call sites.
There was a problem hiding this comment.
+1, any particular reason this was changed?
| fun LoadingScreenSpatial( | ||
| onCancelPress: () -> Unit, | ||
| ) { | ||
| BackgroundSubspace( | ||
| aspectRatio = 1.4f, | ||
| minimumHeight = 500.dp, | ||
| drawable = CreationR.drawable.squiggle_light, | ||
| ) { | ||
| MainPanelWorkaround() | ||
| Orbiter( | ||
| position = ContentEdge.Top, | ||
| offsetType = OrbiterOffsetType.OuterEdge, | ||
| offset = 32.dp, | ||
| alignment = Alignment.End, | ||
| ) { | ||
| RequestHomeSpaceIconButton( | ||
| modifier = Modifier | ||
| .size(64.dp, 64.dp) | ||
| .padding(8.dp), | ||
| colors = IconButtonDefaults.iconButtonColors( | ||
| containerColor = MaterialTheme.colorScheme.secondaryContainer, | ||
| ), | ||
| ) | ||
| } | ||
| SpatialPanel( | ||
| SubspaceModifier | ||
| .fillMaxWidth(squiggleSafeContentWidth) | ||
| .fillMaxHeight(squiggleSafeContentHeight) | ||
| .offset(z = 10.dp), | ||
| ) { | ||
| LoadingScreenScaffold( | ||
| topBar = {}, | ||
| onCancelPress = onCancelPress, | ||
| containerColor = Color.Transparent, | ||
| ) { | ||
| LoadingScreenContents() | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
This composable contains several hardcoded 'magic numbers' for layout properties (e.g., 1.4f, 500.dp, 32.dp, 64.dp, 10.dp). Extracting these into named private constants at the top of the file would significantly improve readability and maintainability. For example: private const val BACKGROUND_ASPECT_RATIO = 1.4f.
2 participants
Adds a
LoadingScreenSpatiallayout for XR devices. The design calls for a different type of background so the component has been made more generic to support different minimum heights, aspect ratios, and background fills.Screenshots