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

Mobile sidebar swipe #8159

Merged
merged 14 commits into from
Jun 6, 2014
Merged

Mobile sidebar swipe #8159

merged 14 commits into from
Jun 6, 2014

Conversation

jancborchardt
Copy link
Member

APP DEVS: If tapping on an element in the app-navigation switches to the content and you don’t want that (for example if it’s a button or so), use .app-navigation-noclose on the highest element in which you do not want taps to close the navigation.


Fixing #176

This will make ownCloud usable on mobile again since we introduced sidebars. Specifically this is about having a way to toggle between the sidebar (app-navigation) and content (app-content).

This is done using Snap.js and works via swipe as well as with the button in the top left.

TODO:

  • settle on a way to do the sidebar – FIXED, YEAH! Thanks to everyone! :)
  • switch to the app-content when an entry in app-navigation is clicked (for navigation)
  • decide where to put a toggle button (using the menu.svg icon) so it’s more discoverable than just swiping
  • test if drag&drop works, maybe adjust so both swiping and drag&drop works
  • the hard part: adjust the apps which use app-navigation/app-content but don’t work / modify core apps.css
  • test opening files, for example with the text editor
  • check smoothness of animation against demo at http://jakiestfu.github.io/Snap.js/demo/apps/default.html

@tanghus
Copy link
Contributor

tanghus commented Apr 11, 2014

@jbtbnl for contacts it seems to be this commit owncloud/contacts@0382cee

@jancborchardt
Copy link
Member Author

@jbtbnl I’d be very interested what you think about how to do the sidebar right. I think we do need to position that and the content absolutely. The question is, should we use a fixed 250px width, or percentage? I’d also be cool with fixed, but what do you think about the percentage?

@jbtbnl
Copy link
Contributor

jbtbnl commented Apr 11, 2014

@jancborchardt how did you test the swipe gesture? I'm testing it with Google Chrome device emulation but it won't work...

Edit: works on my BB Q10.

@jbtbnl
Copy link
Contributor

jbtbnl commented Apr 11, 2014

@jancborchardt af few feedback points:

  • the snap.js example on http://jakiestfu.github.io/Snap.js/demo/apps/default.html does work with Chrome device emulation and the animation is a lot smoother (don't know why)
  • the navigation bar should reach all the way to the top so that all vertical space is efficiently used (might improve performance as well)
  • the header should have the "three horizontal lines" menu button on the left so that the user knows that there is a hidden menu. We need to consider how to match this with the apps menu. Having two different menu's can be confusing for the user. Maybe relocate the app icons to the top of the menu on mobile?

Disclaimer: I do realize that it's not an easy job to implement the above mentioned :)

@jbtbnl
Copy link
Contributor

jbtbnl commented Apr 11, 2014

A mock-up of app icons in sidebar on mobile:
untitled

A mock-up with distinction between app-navigation and apps menu:
untitled2

@Niduroki
Copy link
Member

Looks pretty good so far!

@jancborchardt
Copy link
Member Author

@jbtbnl good concepts! I really wonder about the combined sidebar vs the separated one:

  • combined would be better, less cognitive load. We also thought about combining the apps list with the user menu in the past.
  • on the other hand, for people who use more apps, the combined menu does not really scale and pushes the app menu to the bottom

And if we don’t make it combined, it would be confusing to have the sidebar be full height because that makes it seem global. It’s not though. It’s just a view on an app, and some apps are not going to need it either.

(Also, in the mockups, make sure to not miss the ownCloud logo and the user menu. ;)

@jancborchardt
Copy link
Member Author

@jbtbnl btw I would really like to have a call with you about that. Do you use Skype, or wanna use http://talky.io or whatnot? I’m busy until Sunday but Monday would work. (We can talk about it on IRC.)

@jbtbnl
Copy link
Contributor

jbtbnl commented Apr 11, 2014

@jancborchardt yeah, I forgot about the user menu, the ownCloud logo not so much :)
I'll contact you about the video call.

@jbtbnl
Copy link
Contributor

jbtbnl commented Apr 14, 2014

mobile-mockup-2

@PVince81
Copy link
Contributor

Please make sure to also test opening files, for example with the text editor: it will auto-hide the sidebar to display itself fullscreen (since the sidebar PR). I guess it should still work with your approach, but better be sure 😄

@jancborchardt
Copy link
Member Author

Updated, please test this out on mobile and desktop @owncloud/designers!

@jancborchardt jancborchardt added this to the ownCloud 7 milestone Jun 5, 2014
@jancborchardt jancborchardt changed the title [WIP] Mobile sidebar swipe Mobile sidebar swipe Jun 5, 2014
@LEDfan
Copy link
Member

LEDfan commented Jun 5, 2014

I have 3 remarks/questions:

  • it works very well and is a nice very needed function!
  • it breaks the contacts app on mobile and a bit on dekstop (probably I'll try to fix this)
  • If you click on a group (in contacts app)/news item (in news)/conv (in chat app) is it supposed to close the app-navigation?

@ghost
Copy link

ghost commented Jun 5, 2014

💣 Test Failed. 💣
Refer to this link for build results: https://ci.owncloud.org/job/pull-request-analyser/5385/

@jbtbnl
Copy link
Contributor

jbtbnl commented Jun 5, 2014

If you click on a group (in contacts app)/news item (in news)/conv (in chat app) is it supposed to close the app-navigation?

@LEDfan yes, I think that is the desired behavior.

@jancborchardt
Copy link
Member Author

One thing which still needs to be fixed: The content is swipable to the right even when you are not in mobile mode. This results in big whitespace.

@MorrisJobke @jbtbnl could you check if you can fix that? Maybe we need to wrap the snap.js part in js.js in a media query. Or disable something in the call to snap.js

@ghost
Copy link

ghost commented Jun 5, 2014

💣 Test Failed. 💣
Refer to this link for build results: https://ci.owncloud.org/job/pull-request-analyser/5417/

@jbtbnl
Copy link
Contributor

jbtbnl commented Jun 5, 2014

@jancborchardt there still are major compatibility issues with all apps and the performance is terrible. I understand that you want to push this before the feature freeze but in this state it hardly fixes anything...

@MorrisJobke
Copy link
Contributor

I fixed the problem with wider screens (now it's just activated for small screens (< 768px)

@ghost
Copy link

ghost commented Jun 6, 2014

💣 Test Failed. 💣
Refer to this link for build results: https://ci.owncloud.org/job/pull-request-analyser/5421/

@scrutinizer-notifier
Copy link

The inspection completed: 22 new issues

@PVince81
Copy link
Contributor

PVince81 commented Jun 6, 2014

  • auto-close when window resize

@PVince81
Copy link
Contributor

PVince81 commented Jun 6, 2014

  • debounce resize event

@scrutinizer-notifier
Copy link

A new inspection was created.

@PVince81
Copy link
Contributor

PVince81 commented Jun 6, 2014

👍

@jancborchardt
Copy link
Member Author

Ok, changes by you look good as well @PVince81 👍

@jancborchardt
Copy link
Member Author

JS tests run locally

@MorrisJobke
Copy link
Contributor

@jancborchardt Also document this!!! Otherwise it will just be lost and only used in some apps

@ghost
Copy link

ghost commented Jun 6, 2014

💣 Test Failed. 💣
Refer to this link for build results: https://ci.owncloud.org/job/pull-request-analyser/5431/

@DeepDiver1975
Copy link
Member

failing unit tests are unrelated -> merge

DeepDiver1975 added a commit that referenced this pull request Jun 6, 2014
@DeepDiver1975 DeepDiver1975 merged commit 289accc into master Jun 6, 2014
@DeepDiver1975 DeepDiver1975 deleted the mobile-sidebar-swipe branch June 6, 2014 09:27
@jancborchardt
Copy link
Member Author

previewtemplate_display

@@ -1131,6 +1131,60 @@ function initCore() {
}

setupMainMenu();

// just add snapper for logged in users
if($('#body-login, #body-public').length === 0) {
Copy link
Contributor

Choose a reason for hiding this comment

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

@jancborchardt didn't you change this to #app-navigation ? I remember you did when we were having a look at it.
Either you forgot to push that change or reverted back ?

@lock lock bot locked as resolved and limited conversation to collaborators Aug 16, 2019
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.

None yet