Skip to content
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

Merged
merged 3 commits into from
Jul 3, 2024

Conversation

MichaelBuessemeyer
Copy link
Contributor

@MichaelBuessemeyer MichaelBuessemeyer commented Jun 28, 2024

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):

  • https://___.webknossos.xyz

Steps to test:

  • Open the login view of the dev instance. Zoom the page out. The login form should always be horizontally centered. Compare it to the current master version where the login form is not always centered.
  • Create an annotation of a non-public dataset. Then, share this annotation as public, copy the link and log out from the dev instance. Open the annotation. The annotation should not be opened as the underlying dataset it not public instead the other version of the login form should be rendered. This login form should be center horizontally as well regardless of the zooming :)

Issues:

  • No issue exists for this

(Please delete unneeded items, merge only when none are left open)

@@ -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" }}>
Copy link
Member

@hotzenklotz hotzenklotz Jun 28, 2024

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?

Copy link
Contributor Author

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.

Copy link
Member

@hotzenklotz hotzenklotz Jul 1, 2024

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" }}>
Copy link
Member

Choose a reason for hiding this comment

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

display: "table"

🙈

Copy link
Contributor Author

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 🤷‍♂️

Copy link
Member

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.

Copy link
Contributor Author

@MichaelBuessemeyer MichaelBuessemeyer left a 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?

@MichaelBuessemeyer MichaelBuessemeyer merged commit 641c88d into master Jul 3, 2024
2 checks passed
@MichaelBuessemeyer MichaelBuessemeyer deleted the fix-login-from-centering branch July 3, 2024 07:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants