Skip to content

Conversation

@skjnldsv
Copy link
Member

@skjnldsv skjnldsv commented Nov 22, 2017

This allows the browser to automatically compute the full content.
Everything is center except the footer as long as the window as enough space to display everything.
The fallback in case of big content or small screen is the reducing of the blank space between the man content and the top and the main content and the footer. After that, the total height is expanded by a scroll.

Examples with various sizes:
dev0dev1dev2

Update example (very big picture)

Signed-off-by: John Molakvoæ (skjnldsv) <[email protected]>
Signed-off-by: John Molakvoæ (skjnldsv) <[email protected]>
@codecov
Copy link

codecov bot commented Nov 22, 2017

Codecov Report

Merging #7249 into master will increase coverage by <.01%.
The diff coverage is n/a.

@@             Coverage Diff              @@
##             master    #7249      +/-   ##
============================================
+ Coverage     50.84%   50.84%   +<.01%     
  Complexity    24548    24548              
============================================
  Files          1585     1585              
  Lines         93804    93804              
  Branches       1354     1354              
============================================
+ Hits          47693    47698       +5     
+ Misses        46111    46106       -5
Impacted Files Coverage Δ Complexity Δ
core/js/js.js 63.55% <0%> (+0.56%) 0% <0%> (ø) ⬇️

@jancborchardt
Copy link
Member

On small screens it seems to be a bit too high, no?
screenshot from 2017-11-22 18-34-09
Compared to master, where it has less distance to the footer, nicer spacing:
screenshot from 2017-11-22 18-37-17

Also the issue we have on master since the vertical centering change is still present. When the height of the content changes, the alignment changes. This makes the content move around vertically in cases like an update where the content changes very quickly. You can easily test it with the »Forgot password« view where the content is shorter:
screenshot from 2017-11-22 18-34-36

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.

See comment :)

@skjnldsv
Copy link
Member Author

skjnldsv commented Nov 22, 2017

Okay, it's an easy fix, but we need to ask ourselves: (@nextcloud/designers)

  1. Do we want responsiveness, in that case always display the maximum data we can and scroll only if really necessary?
    OR
  2. Do we want regularity, where the logo is always at the same place but where we can have some stuff like this in small screens:
1 2
dev skjnldsv com_40402_login 1 dev skjnldsv com_40402_login 2

Ps: yes, it's me on the pictures

@jancborchardt
Copy link
Member

Before we had both, or a better inbetween. A constant position of the logo for changing content, but also good position in general.

Sure in theory vertical alignment is great. But of in practice it has these issues does it make more sense to revert to before?

@pixelipo
Copy link
Contributor

Perhaps we should introduce media queries...

@jancborchardt
Copy link
Member

The problem is independent of the screen size. :) When the content is vertically centered but then the content changes quickly (like on update), or even slower (like on Forgot password) everything moves around.

That's why we need to make sure the top part of the content (like the logo) is always at the same position. That's why it was set with a margin-top set to a percentage before. Sure it's not absolutely perfect and not vertically centered, but it's a better working solution.

@skjnldsv @pixelipo except if you say there's an easy way to achieve the same. But right now it seems we are adding complexity on top of complexity to make a fairly simple case work. ;)

@skjnldsv
Copy link
Member Author

skjnldsv commented Nov 22, 2017

@jancborchardt I don't like absolute positionning. This is a pain in the ass and we should never use it for main contents. That being said, going to flex and margin is the solution. We can indeed just put the first margin top to a specific value like 00vh or stuff that depends on the full screen height so we don't have a jump like you say :)

PS: current master has the jump like you said, so either way, this pr is not the problem here ;)

@Henni
Copy link
Member

Henni commented Nov 22, 2017

Is it feasible to modify the html as well or are there to many pages that would be affected?
In case it is possible to also modify the html, I might have an idea how we could solve this:
https://codepen.io/Henni/pen/MOGEWB

@skjnldsv
Copy link
Member Author

@Henni yes it is. But to be fair I was considering standardizing the guest page for 14. With a flex column. Right now the idea was just to go for a quick removal of the absolute positioning :)

Signed-off-by: John Molakvoæ (skjnldsv) <[email protected]>
@skjnldsv
Copy link
Member Author

skjnldsv commented Nov 23, 2017

@jancborchardt Should be better now :)

kazam

@jancborchardt
Copy link
Member

@skjnldsv yes, master has the issue too because #6736 introduced it. ;) It also caused another issue as commented by @ChristophWurst in there too.

That's why we have to go with a solution we can backport to stable13 now e have feature freeze, without breaking much.

That said, looks much better now! :) 👍

@skjnldsv
Copy link
Member Author

I'll fix the totp in another pr. Please review now! :)

@pixelipo
Copy link
Contributor

OK, here's my review opinion. I'm adding it as a comment since it's not breaking issues, but I would like to see these fixed:

  1. Logo is too high up; actually, its distance from top is OK, but now it's too far from the form itself:
    image
    I would remove 16px top+bottom margins from form {} in guest.css since they don't seem needed

  2. Is there a reason why footer has height: 70px? We can easily remove it (I think) and change the p.info {margin: 0 auto; padding-top: 20px; to simpler p.info {margin: 20px auto;} Added advantage to these changes would mean that the whole page would fit nicely on 320*480 screen:
    image

  3. "Wrong password " warning is above the #submit# button, but the #lost-password` warning is bellow - makes no sense at all:
    image

  4. Additionally, "Lost Password" warning also has opacity: .7; added to it for some reason.

@jancborchardt
Copy link
Member

@pixelipo agreed with all of those, however all those we should do separately as we need something minimally invasive we can use for master now cause it’s feature freeze. :)

Maybe the easiest is still to revert #6736@MorrisJobke @skjnldsv?

@skjnldsv
Copy link
Member Author

@jancborchardt how is the update screen with the current master? I can't test right now :/

@pixelipo
Copy link
Contributor

OK, I've copied my comment into a new issue: #7265

As far as I'm concerned, we should merge this as I see no other problems

@jancborchardt
Copy link
Member

This is how it looks like on a big screen or high resolution:
screenshot from 2017-11-25 15-40-02

@skjnldsv it seems your last commit did that, and the 10% is not even used.

So, I would say now we should revert #6736 to fix master/stable13, and then we can fix it properly fix it in master going forward?

@skjnldsv
Copy link
Member Author

@jancborchardt it does not on my side. @MorrisJobke is this the same for you?

@MorrisJobke
Copy link
Member

@jancborchardt it does not on my side. @MorrisJobke is this the same for you?

Not really - it is not fully centered but maybe at 1/3 to 1/4 depending on the zoom level (in the middle when zoomed in, between middle and a third when in normal mode and between a quarter and a third when zoomed out).

@skjnldsv
Copy link
Member Author

@MorrisJobke that's how it's supposed to be then. @jancborchardt could you try again? :)

@skjnldsv
Copy link
Member Author

skjnldsv commented Dec 6, 2017

Shameless bump!

@skjnldsv skjnldsv mentioned this pull request Dec 8, 2017
28 tasks
@MorrisJobke MorrisJobke added this to the Nextcloud 13 milestone Dec 8, 2017
Copy link
Member

@rullzer rullzer left a comment

Choose a reason for hiding this comment

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

Fine by me

@MorrisJobke
Copy link
Member

Let's go with this for now. @jancborchardt Please have another look how it works now and then we look why it works for us and not on your system. Merging ...

@MorrisJobke MorrisJobke merged commit 5bc8c94 into master Dec 11, 2017
@MorrisJobke MorrisJobke deleted the guest-flex branch December 11, 2017 14:41
@jancborchardt
Copy link
Member

Works fine in Chrome (left) but the issue persists in Firefox (incognito window, cache cleared, latest master). Can no one reproduce?
screenshot from 2017-12-11 23-52-56

@MorrisJobke
Copy link
Member

Works fine in Chrome (left) but the issue persists in Firefox (incognito window, cache cleared, latest master). Can no one reproduce?

After a reinstall and refresh it now also shows up in Firefox :/

@skjnldsv Could you have a look?

@skjnldsv
Copy link
Member Author

Taking a look right now!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

3. to review Waiting for reviews design Design, UI, UX, etc. enhancement low

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants