Skip to content
This repository has been archived by the owner on Nov 8, 2018. It is now read-only.

Fix button and sidebar layout #1476

Merged
merged 3 commits into from
May 5, 2016
Merged

Conversation

skjnldsv
Copy link
Contributor

@skjnldsv skjnldsv commented May 5, 2016

Fix some misused css data and reorder the sidebar layout to have a full width new message button without the need of js.

refs #1463
@ChristophWurst

@skjnldsv skjnldsv self-assigned this May 5, 2016
@mention-bot
Copy link

By analyzing the blame information on this pull request, we identified @jancborchardt, @ChristophWurst and @DeepDiver1975 to be potential reviewers

@ChristophWurst
Copy link
Contributor

bildschirmfoto von 2016-05-05 10-12-23

@ChristophWurst
Copy link
Contributor

bildschirmfoto von 2016-05-05 10-29-38

@ChristophWurst
Copy link
Contributor

:+1:

Tested with FF an Chrome, works perfectly!

@skjnldsv
Copy link
Contributor Author

skjnldsv commented May 5, 2016

Ahahahahahahhahaha! You're the best Christoph! 💯

@ChristophWurst ChristophWurst mentioned this pull request May 5, 2016
@ChristophWurst ChristophWurst merged commit 1036953 into master May 5, 2016
@ChristophWurst ChristophWurst deleted the fix-button-and-sidebar-layout branch May 5, 2016 08:52
@ChristophWurst ChristophWurst added this to the 0.4.4 milestone May 5, 2016
@jancborchardt
Copy link
Contributor

Yooo, have yet to test cause I'm on mobile but that looks pretty damn cool! :) Great work

@ChristophWurst
Copy link
Contributor

@jancborchardt I thought you have your dev env on the smartphone? :P

@jancborchardt
Copy link
Contributor

Hmmm, the problem though is that when there’s a scrollbar (like when you expand the collapsed folders), that scrollbar will be obstructed by the New Message button background … ;) Which is exactly why the hackery was in place. @skjnldsv

@skjnldsv
Copy link
Contributor Author

skjnldsv commented May 6, 2016

@jancborchardt I don't have this problem at all on my device :)
screenshot_2016-05-06-11-35-13

@skjnldsv
Copy link
Contributor Author

skjnldsv commented May 6, 2016

The new message is out of the scroll section, so the scrollbar will be contained to the list only, no risk of it being hidden by an other element.

@jancborchardt
Copy link
Contributor

That was the visual point though, to have it in the same area. Now the sidebar feels cramped cause the list is cut off both at top and bottom. :/

@ChristophWurst
Copy link
Contributor

I don't think it looks cramped because mostly there are only a few items in there as the folders are collapsed by default now :-/

@skjnldsv
Copy link
Contributor Author

skjnldsv commented May 6, 2016

The sidebar is also a visual indicator.
Using a full height sidebar for a no-full-height element is not a good idea. When i see the scrollbar I assume that I can scroll from the start to the bottom. And using fixed elements like that in the css and in the global layout is also bad. (see owncloud/core#22347)

You should only have a scrollbar on scrollable elements, not just on a global div which only move a part of the page.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants