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

Fix app menu calculation for random size of the right header #10890

Merged
merged 1 commit into from
Nov 3, 2018

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.

@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.

5 participants