-
Notifications
You must be signed in to change notification settings - Fork 22
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
(Re-)Center login forms [not urgent] #7909
Conversation
@@ -30,7 +30,7 @@ function LoginView({ history, redirect }: Props) { | |||
return ( | |||
<Row justify="center" align="middle" className="login-view"> | |||
<Col xs={22} sm={20} md={16} lg={12} xl={8}> | |||
<Card className="login-content"> | |||
<Card className="login-content" style={{ margin: "0 auto" }}> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Die row
in Zeile 31 centered
doch schon den content. Ich versteh das nicht?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The row centers its children, thats correct. Thus, the column is centered. But the Card (child of the column) does not fill the columns full width in case of large screens. In that scenario the Card just starts at the left edge of the column and is therefore not centered on large screens. Thist margin auto fix, mitigates this and the result is a centered login view.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right. In that case I would add the margin: auto
style to the login-content
CSS class/Less file directly. That way "Sign up" and "Reset Password" also profit from theses changes.
EDIT: I included this suggestion in my proposal commit ff14f5e
@@ -107,8 +107,10 @@ export function CoverWithLogin({ onLoggedIn }: { onLoggedIn: () => void }) { | |||
align="middle" | |||
> | |||
<Col xs={22} sm={20} md={16} lg={12} xl={8}> | |||
<h3>Try logging in to view the dataset.</h3> | |||
<LoginForm layout="horizontal" onLoggedIn={onLoggedIn} /> | |||
<span style={{ margin: "0 auto", display: "table" }}> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
display: "table"
🙈
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🙈
Just my thought 😆. But this was the only CSS fix I could come up with in a reasonable amount of time. Somehow this centers the the whole login form 🤷♂️
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I propose to style this view identical to the regular login view. That way we can 1) reuse styling classes, 2) use the same nice design/background, and 3( avoid the weird table
stuff.
See my commit ff14f5e for a proposal.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome @hotzenklotz. Thanks for pushing the changes. I just tested them and they are working splendid. Could you please approve this pr to get this merged?
Due to the recent PR #7876 making wk a little more accessible for mobile devices, the login forms were not centered anymore on devices with a high screen width. This PR aims to fix this.
URL of deployed dev instance (used for testing):
Steps to test:
Issues:
(Please delete unneeded items, merge only when none are left open)