Skip to content

Conversation

Annaseli
Copy link
Contributor

@Annaseli Annaseli commented Aug 13, 2025

Closes #9430

Change Description

Description

This PR previously included changes that I moved to this PR. It now contains only appearance-related updates and TypeScript error fixes. The comments provided here have been addressed in the other PR.

  • Fixed TypeScript errors in login.tsx.
  • Updated the appearance of the login page:
    • Removed the simplified top navbar when the user is logged out.
    • Added a lakeFS logo inside the login modal.
    • Centered the login modal on the page.
    • Centered the login title within the modal.

Old lakeFS login page:
old_login_page

New lakeFS login page:
Image

@Annaseli Annaseli added the include-changelog PR description should be included in next release changelog label Aug 13, 2025
@Annaseli Annaseli requested review from a team August 13, 2025 20:02
Copy link
Contributor

@yonipeleg33 yonipeleg33 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Partial review, of course. I don't have enough context to approve


class DefaultLoginMethodSelectionPlugin implements PluginLoginMethodSelection {
// eslint-disable-next-line @typescript-eslint/no-unused-vars
renderMethodSelection(_loginConfig: LoginConfig | undefined): React.ReactElement | null {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Following my proposal in the other PR,

Suggested change
renderMethodSelection(_loginConfig: LoginConfig | undefined): React.ReactElement | null {
renderLoginMethod(_loginConfig: LoginConfig | undefined): React.ReactElement | null {

And plugins will check the method string to know which component to return

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@yonipeleg33
thanks I renamed it to: renderLoginMethodComponent
and the Plugin name to: PluginLoginMethod
and the default plugin imp to: DefaultLoginMethodPlugin

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@yonipeleg33

This PR previously included changes that I moved to this PR. It now contains only appearance-related updates and TypeScript error fixes. The comments provided here have been addressed in the other PR.

Comment on lines 140 to 141
if (router.query.method === 'local' && loginConfig) {
return <LoginForm loginConfig={loginConfig}/>;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you can ensure LoginConfig isn't false-y, remove | undefined from the plugin's signature

Copy link
Contributor

@itaigilo itaigilo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice plugin design!

I'd like to see the Enterprise part before completing this review, as some context is missing in this case.

Plus, blocking for the (info) comment atm.

@@ -64,39 +64,31 @@ const TopNavLink = ({ href, children }) => {
};

const TopNav = ({logged = true}) => {
if (!logged) {
if (logged) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What happens if !logged?
There's no Navbar? How does the user logs in in this case?

Copy link
Contributor Author

@Annaseli Annaseli Aug 14, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@itaigilo
The TopNav component refers to the black navigation bar at the top, as shown here:
old_login_page

In the new login page, when !logged, TopNav will return nothing, meaning the black top navigation bar will not be displayed.
The only element visible when !logged will be the Login modal, as seen here:
new_login_page

But as requested by Barak, I’ll split this PR to two PR, this one will handle the design changes and the other will cover the plugin functionality.

Copy link
Contributor Author

@Annaseli Annaseli Aug 14, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@itaigilo

This PR previously included changes that I moved to this PR. It now contains only appearance-related updates and TypeScript error fixes.

Copy link
Contributor

@nopcoder nopcoder left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it possible to split the UI change with the functionality to extend. It will enable a quicker way to approve changes.

@Annaseli
Copy link
Contributor Author

The UI feature I was referring to in the description of this PR is linked here.

@Annaseli
Copy link
Contributor Author

Is it possible to split the UI change with the functionality to extend. It will enable a quicker way to approve changes.

@nopcoder
sure i'll split it now.

@Annaseli Annaseli changed the title Added plugin support for login method selection in Enterprise and updated login page appearance Updated login page appearance Aug 14, 2025
@Annaseli
Copy link
Contributor Author

This PR previously included changes that I moved to this PR. It now contains only appearance-related updates and TypeScript error fixes.

Copy link
Contributor

@itaigilo itaigilo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice cleanup,
I like this move.

This can / should be cleaned-up and simplified though.
Plus, the image currently looks squashed 🤷

.login-logo {
width: 170px;
height: 60px;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These are wrong values:
The image itself is 359x82 (if I'm looking at the right one),
Plus, the image looks squashed horizontally.


.login-container {
height: 100vh;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is vh?
Do we use it anywhere else?

} catch(err) {
if (err instanceof AuthenticationError && err.status === 401) {
const contents = {__html: `${loginConfig.login_failed_message}` ||
<div className="d-flex align-items-center justify-content-center login-container">
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The login-container (and its css) isn't needed, and positioning can / should be simplified.

Just define for the login-card:
margin: 50px auto auto auto

It will place it at a fixed margin from the top, and center it horizontally.

And in addition, vertical centering is tricky with html + css, and prone for errors.

if (err instanceof AuthenticationError && err.status === 401) {
const contents = {__html: `${loginConfig.login_failed_message}` ||
<div className="d-flex align-items-center justify-content-center login-container">
<Card className="login-widget shadow-lg border-0 login-card">
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why both login-widget and login-card?

<div className="mb-3">
<img src="/logo.png" alt="lakeFS" className="login-logo" />
</div>
<h4 className="mb-0">Login</h4>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is more for @nopcoder , I guess, but -
There's already a button that says Login,
So there's no need to repeat it (is creates an unnecessary cognitive load on the user).
IMO this h4 can simply be removed.

@nopcoder nopcoder removed their request for review August 20, 2025 08:35
@nopcoder nopcoder dismissed their stale review August 20, 2025 08:37

remove myself as I wanted to verify this change does not include the functionality related to the login selection.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
include-changelog PR description should be included in next release changelog
Projects
None yet
Development

Successfully merging this pull request may close these issues.

To change the appearance of the login page
4 participants