Skip to content

Conversation

@susnux
Copy link
Contributor

@susnux susnux commented May 8, 2023

Summary

  1. Make the background of the login form look like a dashboard panel.
  2. Add the same background style to the footer so it is independent of the background image / color and fulfill the contrast requirements.

Screenshots

what before after
shot footer Screenshot 2023-05-09 at 00-36-55 Nextcloud image
long footer Screenshot 2023-05-09 at 00-38-35 Nextcloud image
color: white; background: black;¹ Screenshot 2023-05-09 at 00-37-26 Nextcloud image

¹: Like the issue: if the user agent uses a custom default browser theme

Checklist

@susnux susnux added design Design, UI, UX, etc. 3. to review Waiting for reviews accessibility labels May 8, 2023
@susnux susnux requested review from a team, Pytal, jancborchardt, nfebe, nimishavijay and szaimen and removed request for a team May 8, 2023 22:56
@solracsf
Copy link
Member

solracsf commented May 9, 2023

IMO, the same type of border-radius should be applied.

image

@szaimen
Copy link
Contributor

szaimen commented May 9, 2023

Yes, I would recommend the same corner roundness as well and maxbe also make it the same width like the login background?

@susnux
Copy link
Contributor Author

susnux commented May 9, 2023

@szaimen @solracsf there was a design discussion in the linked issue and also here #35482 (comment), so I just picked the pill style as it seems to be the preferred one (?).

maxbe also make it the same width like the login background?

I do not think that works, because the footer text is much wider, looks like this:

image

@solracsf
Copy link
Member

solracsf commented May 9, 2023

No problem, but from a design perspective, it makes no sense (to me) to have multiple border radius ;-)

@nimishavijay
Copy link
Member

I would vote for pill style too, so that it reflects how it looks in the dashboard (border-radius-large boxes, and the smaller buttons are pill shaped). What do you think? And agreed about the width, it might make the height too big, especially for translations.

What do you think about the existing padding being reduced such that the height of the div is 44px? It could look like this:

@szaimen
Copy link
Contributor

szaimen commented May 9, 2023

with reduced padding it indeed looks much better 👍

@susnux
Copy link
Contributor Author

susnux commented May 11, 2023

@nimishavijay is it very important that the footer is not wider than the login box?
Because for English it works, but most other languages I tried (French, Spanish, Portuguese, German) require more space (about 10 to 30px more).

I personally think that less height but wider footer look better:

Spanish (min-width: 300px) Portuguese (width: 300px)
image image

And for the bright color theme

French (min-width: 300px) French (width: 300px)
image image

@susnux susnux force-pushed the fix/login-footer-design branch from ddfbf6f to 05a0606 Compare May 11, 2023 10:55
@nimishavijay
Copy link
Member

Because for English it works, but most other languages I tried (French, Spanish, Portuguese, German) require more space

Oh it was a misunderstanding! I agreed with your point about the width, so it doesn't need to be limited :)

Copy link
Member

@nimishavijay nimishavijay left a comment

Choose a reason for hiding this comment

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

Latest screenshots look great! Good to go design-wise :) 🚀

Copy link
Member

@jancborchardt jancborchardt left a comment

Choose a reason for hiding this comment

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

Looks great @susnux! :) I’d actually say we could use an even larger border-radius for the login box as well – the same as for the content when logged in: var(--body-container-radius). What do you think?

@susnux
Copy link
Contributor Author

susnux commented May 11, 2023

I’d actually say we could use an even larger border-radius for the login box as well – the same as for the content when logged in: var(--body-container-radius). What do you think?

@jancborchardt
I intended to make it consistent with the dashboard panels, which use --border-radius-large:

border-radius: var(--border-radius-large);

As the login box is more a panel than the main content box (?)

@jancborchardt
Copy link
Member

jancborchardt commented May 11, 2023

Right, that makes sense @susnux – considering that the Dashboard panels are older than the redesign though, I’d say it’s better to align with the redesign. :) Especially since the new added container around the footer looks rounder than the login form container otherwise.

And actually the dashboard panels also look better with the bigger border-radius (var(--body-container-radius) corresponds to 28px), and work better with the internal radii of the entries.

Login screen with bigger border radius Dashboard panel with bigger border radius
image image

@susnux
Copy link
Contributor Author

susnux commented May 15, 2023

Looks great! I modified the PR accordingly, the dashboard will be a follow up PR to keep it small :)
(But why does the variable does not match the schema? Like --border-radius-XY, just feels a bit strange ^^ )

image

@susnux susnux force-pushed the fix/login-footer-design branch 2 times, most recently from 505bd97 to b9c2d2a Compare May 15, 2023 10:17
szaimen

This comment was marked as resolved.

@susnux susnux force-pushed the fix/login-footer-design branch from b9c2d2a to 3d245a8 Compare May 15, 2023 14:03
@susnux
Copy link
Contributor Author

susnux commented May 15, 2023

@szaimen yes, cache issue... Happens because that css variable is set to 0px for mobile view. Fixed it.

As the variable does not exist on mobile I moved it to a new one (--border-radius-rounded) so we can use it also on mobile and toggle the body-container-radius between 0px and var(--border-radius-rounded). Maybe @jancborchardt rereview that part?

Copy link
Contributor

@szaimen szaimen left a comment

Choose a reason for hiding this comment

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

Tested and seems to work now but didnt review the code

Copy link
Member

@jancborchardt jancborchardt left a comment

Choose a reason for hiding this comment

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

Seems good to me! And now looks very nice with the 2 similar roundings. :) Nice job @susnux

susnux added 2 commits May 16, 2023 19:47
Also adjust border radius to match new main content box.

Signed-off-by: Ferdinand Thiessen <[email protected]>
…ner-radius` can be used on mobile devices

Signed-off-by: Ferdinand Thiessen <[email protected]>
@susnux susnux force-pushed the fix/login-footer-design branch from 3d245a8 to 6ce567e Compare May 16, 2023 17:51
Signed-off-by: Ferdinand Thiessen <[email protected]>
@susnux susnux force-pushed the fix/login-footer-design branch from 6ce567e to 79239ea Compare May 16, 2023 20:31
@susnux susnux added 4. to release Ready to be released and/or waiting for tests to finish and removed 3. to review Waiting for reviews labels May 16, 2023
@szaimen
Copy link
Contributor

szaimen commented May 16, 2023

CI failure unrelated

@szaimen szaimen merged commit f01d30a into master May 16, 2023
@szaimen szaimen deleted the fix/login-footer-design branch May 16, 2023 21:36
@szaimen szaimen added this to the Nextcloud 27 milestone May 16, 2023
@blizzz blizzz mentioned this pull request May 17, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

4. to release Ready to be released and/or waiting for tests to finish accessibility design Design, UI, UX, etc.

Projects

None yet

6 participants