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

Increase html body height to full content height #22614

Merged
merged 1 commit into from
Sep 10, 2020

Conversation

raimund-schluessler
Copy link
Member

@raimund-schluessler raimund-schluessler commented Sep 5, 2020

This PR increases the html body height to match the full content height. Fixes #22606 and nextcloud-libraries/nextcloud-vue#1384.

This change is necessary, so that the popover boundaries are calculated correctly. Otherwise the every popover scrolls out of the viewport because body is to small.

Before:
92301581-3baab500-ef65-11ea-9a26-d9822ed4f9c5

After:
after

Although code-wise this is a small change, I am a bit afraid of breaking anything, since this affects more or less the root element of the website. I did some testing and didn't find anything which is broken by this change. However, this really needs a good test.

The fluttering of the status popover is due to the container being the body element. This can be fixed by choosing e.g. the header once nextcloud-libraries/nextcloud-vue#1389 is merged. I will do so in a follow up PR.

Also see nextcloud-libraries/nextcloud-vue#1384 for details.

@faily-bot
Copy link

faily-bot bot commented Sep 5, 2020

🤖 beep boop beep 🤖

Here are the logs for the failed build:

Status of 32689: failure

mariadb10.4-php7.3

Show full log
There were 2 warnings:

1) Test\Files\ViewTest::testRenameFailDeleteTargetKeepSource
Trying to configure method "writeStream" which cannot be configured because it does not exist, has not been specified, is final, or is static

2) Test\Files\ViewTest::testCopyFailDeleteTargetKeepSource
Trying to configure method "writeStream" which cannot be configured because it does not exist, has not been specified, is final, or is static

--

There was 1 failure:

1) Test\Files\ObjectStore\ObjectStoreStorageTest::testCopy with data set #7 ('/single ' quote.txt', '/tar'get.txt')
Expected /tar'get.txt to be a copy of /drone/src/tests/data/lorem.txt
Failed asserting that false matches expected 'Lorem ipsum dolor sit amet, consectetur adipisicing elit, sed do eiusmod tempor incididunt ut labore et dolore magna aliqua.\n
Ut enim ad minim veniam, quis nostrud exercitation ullamco laboris nisi ut aliquip ex ea commodo consequat.\n
Duis aute irure dolor in reprehenderit in voluptate velit esse cillum dolore eu fugiat nulla pariatur.\n
Excepteur sint occaecat cupidatat non proident, sunt in culpa qui officia deserunt mollit anim id est laborum.'.

/drone/src/tests/lib/Files/Storage/Storage.php:222
/drone/src/tests/lib/Files/Storage/Storage.php:235

@nickvergessen
Copy link
Member

This is a breaking change, right? Too close to a release for me in that case

@skjnldsv skjnldsv modified the milestones: Nextcloud 20, Nextcloud 21 Sep 5, 2020
@raimund-schluessler
Copy link
Member Author

raimund-schluessler commented Sep 6, 2020

This is a breaking change, right? Too close to a release for me in that case

I don't know if it is breaking. But since master is already broken (you can't open any popover while scrolled down, including the status selection), I think a solution is necessary.

I am happy to see any other proposal.

Copy link
Member

@skjnldsv skjnldsv left a comment

Choose a reason for hiding this comment

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

Couldn't find any bad outcome so far! 🤷

@skjnldsv skjnldsv added 4. to release Ready to be released and/or waiting for tests to finish and removed 3. to review Waiting for reviews labels Sep 7, 2020
@MorrisJobke MorrisJobke merged commit 7fd505f into master Sep 10, 2020
@MorrisJobke MorrisJobke deleted the fix/22606/popover-boundaries branch September 10, 2020 07:34
@MorrisJobke MorrisJobke mentioned this pull request Sep 10, 2020
13 tasks
@nickvergessen
Copy link
Member

Breaks the dashboard:
Bildschirmfoto von 2020-09-10 13-18-22

@nickvergessen
Copy link
Member

As said above, I vote to not merge this and have a look after 20 is out.

@nickvergessen
Copy link
Member

Also breaks Talk and mostlikely some other apps that add a scroller themselves:

  1. Chat input is missing
  2. Double scrolbar is added.
    Bildschirmfoto von 2020-09-10 13-30-32

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 bug feature: status
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Unable to open status selection when scrolled down
6 participants