Skip to content

Conversation

@juliusknorr
Copy link
Member

Fixes #10857

This PR will ensure that at least 210px of space is kept available for the right header icons. The issue in #10857 was that the number of icons to show was calculated before all right icons where loaded. This leads to different behaviour when resizing the window/loading an app that doesn't have a search.

@juliusknorr juliusknorr added bug 3. to review Waiting for reviews labels Aug 27, 2018
@juliusknorr juliusknorr added this to the Nextcloud 14 milestone Aug 27, 2018
@skjnldsv
Copy link
Member

Couldn't we just make sure the calculation is always done properly?
Or just so that icons always have the same width, so that way, even if they're now loaded, the width would be ok?

@weeman1337
Copy link
Member

As @juliushaertl told me adding icons afterwards is working. It's just for the initial loading.

@skjnldsv
Copy link
Member

Yes, but I don't like having hardcoded values. We shouldn't have to do that :)
Can we watch the header changes with an event, or just add another trigger to ensure we wait for things to be loaded before having to launch the calculation?

@juliusknorr juliusknorr force-pushed the bugfix/10857/app-menu-resize branch from 92543cd to 78d9d56 Compare August 28, 2018 12:27
@juliusknorr
Copy link
Member Author

Yes, but I don't like having hardcoded values. We shouldn't have to do that :)
Can we watch the header changes with an event, or just add another trigger to ensure we wait for things to be loaded before having to launch the calculation?

I have no idea how we could do that properly. The issue is that the two divs in the header (left/right) are resized when some icons are added. Do you have an idea how we could properly calculate the icon count every time @weeman1337 @skjnldsv ?

@weeman1337
Copy link
Member

Would be easier if there is a explicit API for adding removing stuff. Maybe we should go for a fix now and improve the header bar in the future (followup)?

@MorrisJobke MorrisJobke mentioned this pull request Aug 30, 2018
6 tasks
@rullzer
Copy link
Member

rullzer commented Aug 30, 2018

@skjnldsv @juliushaertl can we get a decision here. I have to agree with @weeman1337 to get this in now and fix properly for 15. We are in RC so no huge changes please 😉

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.

LGTM then :)

Copy link
Member

@MorrisJobke MorrisJobke 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 works 👍

@MorrisJobke
Copy link
Member

JSUnit fails:

PhantomJS 2.1.1 (Linux 0.0.0) Core base tests Main menu mobile toggle Clicking menu toggle toggles navigation in FAILED
	Expected false to equal true.
	core/js/tests/specs/coreSpec.js:556:46

@rullzer
Copy link
Member

rullzer commented Sep 4, 2018

@juliushaertl see #10890 (comment)

@juliusknorr juliusknorr added 2. developing Work in progress and removed 3. to review Waiting for reviews labels Sep 4, 2018
@MorrisJobke
Copy link
Member

@juliushaertl Ping ;)

@juliusknorr juliusknorr force-pushed the bugfix/10857/app-menu-resize branch 3 times, most recently from 90bc047 to ffb3a5d Compare November 2, 2018 14:24
@juliusknorr
Copy link
Member Author

Tests should be fixed, let's see.

Signed-off-by: Julius Härtl <jus@bitgrid.net>
@rullzer rullzer force-pushed the bugfix/10857/app-menu-resize branch from ffb3a5d to a696c01 Compare November 2, 2018 18:57
@rullzer
Copy link
Member

rullzer commented Nov 2, 2018

Seems good right @juliushaertl ?

@juliusknorr juliusknorr added 3. to review Waiting for reviews 4. to release Ready to be released and/or waiting for tests to finish and removed 2. developing Work in progress 3. to review Waiting for reviews labels Nov 3, 2018
@juliusknorr
Copy link
Member Author

Yep, remaining failures are unrelated. 😉

@rullzer rullzer merged commit b978399 into master Nov 3, 2018
@rullzer rullzer deleted the bugfix/10857/app-menu-resize branch November 3, 2018 08:35
@MorrisJobke
Copy link
Member

@juliushaertl Mind to backport this?

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants