UI: Overhaul lakeFS look & feel #9022
Conversation
ozkatz
added
include-changelog
E2E Test Results - DynamoDB Local - Local Block Adapter
|
E2E Test Results - Quickstart
|
There was a problem hiding this comment.
First of all - Great work overall !
This PR brings valuable UX and architectural improvements, and code readability has generally improved compared to the previous structure.
I personally appreciate the shift from a single path to per-file destination :)
Please note that this PR is quite large, and while the changes are valuable and well structured, it makes the review process significantly harder and more time consuming.
In the future, it would be helpful to split such large changes into smaller, focused PRs (e.g. separating UI refactors from logic changes). That would make it easier to review thoroughly and provide better feedback faster.
Here are a few general suggestions for improvement:
-
The
objects.jsxfile is quite large (1000+ lines). Breaking it up could help with testability and maintainability.
Highly recommend splitting this component into smaller subcomponents. -
Since this PR introduces behavioral changes (e.g., per-file destination paths, editable paths, cancellation handling), it’s important to ensure that there will be at least some unit tests / add playwright tests for it.
-
OPTIONAL - consider wrapping the
AbortControllerlogic into a custom hook if similar cancellation patterns are planned elsewhere in the project. For now, it's fine inline.
| <div className="d-flex align-items-center mt-4"> | ||
| <span className="learn-more">Already working with lakeFS?</span> | ||
| <GettingStartedCreateRepoButton | ||
| style={{ padding: 0, width: "auto", marginLeft: "8px", display: "inline-block" }} |
There was a problem hiding this comment.
Suggest adding a class to a css file and using it here
| useEffect(() => { | ||
| setCurrentPath(path) | ||
| setOverallPath(path || "") | ||
| }, [path]) |
There was a problem hiding this comment.
Consider if this effect is needed at all, or if the default value can be handled outside the effect
|
@ozkatz while trying to play around with this branch, I found a bug- the Upload Object button on the Uncommitted Changes page redirects me back to the objects page of the repository. |
|
Addressing the e2e test on a different PR based on this one - #9098. |
|
@ozkatz merged my changes for the UI tests, should be green now. |
Agreed, I did want to have a concrete call to action for empty states, which we didn't have before. Good catch - I fixed the bug. |
There was a problem hiding this comment.
If we're modifying globals.css in this PR, I wanted to share something I learned from @itaigilo: it's best to minimize the use of global CSS and inline styles, and instead rely on built-in Bootstrap classes as much as possible. These classes have already been tested across various screens and scenarios, and they follow established best practices for styling.
From what I understood from Itai, the long-term goal is to remove most of the custom styles in globals.css and replace them with Bootstrap classes. However, I noticed that this PR introduces several new CSS files in the styles directory.
There was a problem hiding this comment.
The purpose of a globals.css is to provide general styling for the app.
So in this sense, the AI took a good step in this direction, leaving here only general elements style (such as h* or body styling, and color-schema), and moving the rest to the "Areas" css files.
These files are tricky from my POV, but I've commented on the auth.css.
| const isAbortedError = (error, controller) => { | ||
| return (error instanceof DOMException && error.name === 'AbortError') || controller.signal.aborted; | ||
| }; |
There was a problem hiding this comment.
When branch protection is on, an error is thrown and this code throw exception while access undefined (controller.signal.aborted) and we get a blank page with exception in the console.
|
The white background needs a fix in the branch protection. 
|
There was a problem hiding this comment.
This is great effort, in the very right direction.
I totally understand the need for a massive change in the UI, and I think we can benefit a lot from it,
And I also think that using AI for this is probably the only way to do it in 2025.
Having said that, this PR introduces two main things we should consider:
- It requires proper testing and verification.
- It introduces a lot of code which is hard to maintain by humans.
I've roughly gone through a chunk of it, finding a lot of things that I wouldn't approve in a PR of a fellow dev.
So if we want to introduce such changes, I think that we should be aware of the consequences.
Do we want the code to be mainly written by AI in the near future?
Are we willing to risk with some fundamental UI bugs resulting from code we don't understand and haven't tested?
Are we ok with massive PRs which are hard to review?
The answer to these questions well might be a "Yes" -
But it should at least be considered.
And again, IMHO -
This is a very motivating step in the very right direction!
| dismissible | ||
| onClose={onDismiss} | ||
| > | ||
| <div className="d-flex align-items-center"> |
There was a problem hiding this comment.
It's very hard to understand the layout from reading this code.
Better add descriptive classes to such elements.
| } | ||
|
|
||
| return ( | ||
| <Alert className={alertClassName} variant="danger">{content}</Alert> | ||
| <Alert className={alertClassName} variant="danger"> |
There was a problem hiding this comment.
This is a copy of the code above. Should be a component.
| @@ -421,8 +452,13 @@ export const ToggleSwitch = ({ label, id, defaultChecked, onChange }) => { | |||
|
|
|||
| export const Warning = (props) => | |||
| <> | |||
| <Alert variant="warning"> | |||
| ⚠ { props.children } | |||
| <Alert variant="warning" className="shadow-sm"> | |||
| </div> | ||
| if (emptyStateComponent) { | ||
| return emptyStateComponent; | ||
| } else { |
There was a problem hiding this comment.
The else can/should be removed.
| @@ -4,6 +4,25 @@ import { createRoot } from 'react-dom/client'; | |||
| import 'bootstrap/dist/css/bootstrap.css'; | |||
| import './styles/globals.css'; | |||
|
|
|||
| // Areas | |||
There was a problem hiding this comment.
I'd expect the Areas css files to be included in the index file of the relevant area.
When all of these are here, it's much easier to leave a lot of unused styles in case of removed / transformed components.
There was a problem hiding this comment.
yes! will be done in another PR, I think this one introduces enough changes as is :)
webui/src/styles/auth.css
Outdated
There was a problem hiding this comment.
These styles create a mix:
We mainly rely on bootstrap for the layout and general look-and-feel of the UI (positioning, sizes, margins, etc.),
But here, a decent amount of structural changes was introduces.
This might work, but:
- It wasn't properly tested on different screen-sizes, browsers, etc (while "plain" bootstrap UI is designed and tested for these).
- It really hard to maintain by humans.
So taking this approach meaning we're compromising these:
- Not properly tested on a wide range of scenarios.
- Hard to maintain by humans.
There was a problem hiding this comment.
True - it was tested on desktop browsers (Chrome, Firefox, Safari). In my defense the existing code is also a mix 🙃. I am aware this is a bad excuse.
There was a problem hiding this comment.
The purpose of a globals.css is to provide general styling for the app.
So in this sense, the AI took a good step in this direction, leaving here only general elements style (such as h* or body styling, and color-schema), and moving the rest to the "Areas" css files.
These files are tricky from my POV, but I've commented on the auth.css.
| padding: 0; | ||
| } | ||
|
|
||
| .navbar > div { | ||
| padding-top: 7px; | ||
| h1, h2, h3, h4, h5, h6 { |
There was a problem hiding this comment.
Why all h* get the same attributes?
I think this is pretty wrong...
Fixed |
|
@idanovo The upload is fine. Maybe you're not updated with the latest changes locally, just pull changes. Although some things I did noticed (@ozkatz):
|
@Ben-El it was @idanovo who spotted the bug (that did exist) it's fixed now :)
Will correct the margins, I agree.
That's not new in this PR so I won't icrease the scope :)
This is because the UI will attempt to render a README.md, if exists, in any directory. This is existing behavior, I didn't touch it. |
@ozkatz I figured it out! |
|
Sorry for the big PR that does a lot of things. Here's a quick recap of what's included:
global.cssinto smaller, easier to maintain files (this alone is responsible for a lot of the diff)Admittedly, some of the CSS changes also come with modifications for the structure of the page which increases bloat on the diff, but they are almost all cosmetic.