fix(ui): Fix version, environment, and uptime visibility in navbar#27558
Open
kiyeonjeon21 wants to merge 2 commits intoprestodb:masterfrom
Open
fix(ui): Fix version, environment, and uptime visibility in navbar#27558kiyeonjeon21 wants to merge 2 commits intoprestodb:masterfrom
kiyeonjeon21 wants to merge 2 commits intoprestodb:masterfrom
Conversation
Contributor
Reviewer's guide (collapsed on small PRs)Reviewer's GuideAdjusts the navbar layout to restore full visibility of version, environment, and uptime by allowing wrapping and giving the version block its own row while preserving cluster tag support. Flow diagram for updated navbar layout and flex behaviorflowchart LR
Navbar[Navbar container]
CollapseDiv["div id=navbar
navbar-collapse collapse min-width-0"]
NavList["ul.nav navbar-nav navbar-right
class: gap-3 justify-content-end align-items-center min-width-0 flex-grow-1
behavior: flex-wrap (default)"]
VersionItem["li.flex-basis-100 min-width-0
Version block occupies full row"]
ClusterInfoDiv["div.navbar-cluster-info"]
VersionLabel["div.uppercase
label: Version"]
VersionValue["div.text id=version-number
title=nodeVersion.version
no text-truncate (full value visible)"]
Navbar --> CollapseDiv --> NavList --> VersionItem --> ClusterInfoDiv --> VersionLabel
ClusterInfoDiv --> VersionValue
subgraph Previous_layout_behavior
OldNavList["ul with flex-nowrap
Items forced on a single line"]
OldVersionItem["li.flex-basis-40
Version shares row with other items"]
TruncatedText["div.text.text-truncate
Version may be truncated"]
end
OldNavList --> OldVersionItem --> TruncatedText
subgraph New_layout_behavior
NewNavList["ul without flex-nowrap
Items can wrap across lines"]
NewVersionItem["li.flex-basis-100
Version takes entire row"]
FullText["div.text (no truncate)
Full version visible"]
end
NewNavList --> NewVersionItem --> FullText
File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Contributor
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- Consider making the new
.flex-basis-100behavior responsive (e.g., only on smaller breakpoints) so that on wider screens the version block doesn’t unnecessarily occupy a full row when there is ample horizontal space. - Removing
flex-nowrapsolves the truncation, but it may introduce awkward multi-line wrapping for the rest of the navbar content; you might want to test and, if needed, constrain wrapping to specific items (like the version block) rather than the entireul.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Consider making the new `.flex-basis-100` behavior responsive (e.g., only on smaller breakpoints) so that on wider screens the version block doesn’t unnecessarily occupy a full row when there is ample horizontal space.
- Removing `flex-nowrap` solves the truncation, but it may introduce awkward multi-line wrapping for the rest of the navbar content; you might want to test and, if needed, constrain wrapping to specific items (like the version block) rather than the entire `ul`.
## Individual Comments
### Comment 1
<location path="presto-ui/src/static/assets/presto.css" line_range="278-280" />
<code_context>
min-width: 0;
}
+.flex-basis-100 {
+ flex-basis: 100%;
+}
+
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Consider aligning `.flex-basis-100` behavior with `.flex-basis-40` by adding `min-width: 0`.
Without `min-width: 0`, elements using `.flex-basis-100` may not shrink properly in flex layouts and could overflow their container. Adding it here would prevent that and keep behavior consistent with the other `flex-basis-*` utilities.
```suggestion
.flex-basis-100 {
flex-basis: 100%;
min-width: 0;
}
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Contributor
|
Please add a release note - or |
Member
|
Hi @kiyeonjeon21, thanks for the changes. I tried out your changes today and here are my thoughts:
How do you think? |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
#26485 introduced
flex-nowrapon the navbar, which causesversion, environment, and uptime to truncate or overlap when
the values are long (e.g.
0.297-uber-native-SNAPSHOT-99dfabf).Removed
flex-nowrapso items can wrap naturally, and gave theversion field
flex-basis: 100%so it sits on its own row. Alsodropped
text-truncatefrom the version div. This brings backthe pre-#26485 layout while keeping cluster tag support.
Resolves: #26895
Test plan
Release Notes