owncloud-archive / news

:newspaper: News app for ownCloud
GNU Affero General Public License v3.0
289 stars 106 forks source link

Pull to refresh? #781

Open colmoneill opened 9 years ago

colmoneill commented 9 years ago

@jancborchardt tells me that this is supposed to be a pull to refresh and not show the button as it is here? screenshot from 2015-05-01 23 05 51

I'm on Firefox 37.0.2 on Ubuntu 14.04

BernhardPosselt commented 9 years ago

This was done because the previous implementation was too annoying. Use a keyboard shortcut to jump to the previous article or page up to reload the feed. You can also press r

jancborchardt commented 9 years ago

What do you mean with »too annoying«? It worked fine for me and I don’t see how it was annoying.

It definitely is annoying now, just shoving the button in your face. @Raydiation it was you who always refers to this blog post: http://zachholman.com/posts/shit-work/ – but needing to manually press this refresh button is really shitty work.

jancborchardt commented 9 years ago

Sidenote: Keyboard shortcuts are only for advanced people. Please remember a lot of normal people who are not total geeks also use ownCloud.

kklem0 commented 9 years ago

echo @jancborchardt here.

kklem0 commented 9 years ago

Is it the implementation or is it the design you are talking about, @Raydiation ?

BernhardPosselt commented 9 years ago

Ok, let me explain:

In the beginning we reloaded immediately after scrolling up again. This was deemed as very annoying by people because they accidentally hit the top. That's why a separate button was introduced.

As for keyboard shortcuts: hitting the previous article shortcut or page up will ensure that you dont accidentally trigger the refresh.

Please read through the pull to refresh issues on github (there are many) to understand the reason why it works this way.

PS: Pull to refresh is a mobile paradigm and does not work well for desktop websites/applications, it needs to be adjusted to mouse and keyboard

kklem0 commented 9 years ago

@Raydiation I think it works great with a good implementation, and I've seen plenty.

BernhardPosselt commented 9 years ago

@jancborchardt please search the bug tracker. It was annoying to people.

BernhardPosselt commented 9 years ago

Also calling it pull to refresh is wrong I think. You are expecting a pull to refresh feature like you're used to from mobile apps but it actually IS NOT pull to refresh, it's scroll up/go to previous to refresh. And it's totally fine because it works well on the desktop, and again: you are usually using mouse and keyboard. Using your mouse to pull down the stuff for pull to refresh is just ultra weird. And I'm frequently using pull to refresh on mobile.

As for mobile: I've tried it numerous times but snap.js is just ultra flaky on mobile, so is the hamburger menu button. Scrolling on mobile causes a very bad user experience and therefore mobile is not a huge priority. We can talk about a mobile specific pull to refresh implementation again if the mobile interface works.

If you came here to look for a pull to refresh implementation for the mail app, look elsewhere.

If you have constructive criticism other than "meh this is shitty" create a separate issue.

BernhardPosselt commented 9 years ago

BTW I know that this might be your first time creating issues for the News app but this specific feature just gets on my nerves A LOT, so don't be discouraged to suggest a different approach in a constructive manner.

jancborchardt commented 9 years ago

@Raydiation so you are the maintainer of the News app and this is your call to make.

However, representing the ownCloud community I’m asking you to respect our ownCloud Code of Conduct. Directly closing the issue immediately after your first answer above as well as locking it very shortly after is very off-putting to other community members and potential contributors. This is not how we deal with people here – if you’re personally annoyed with a specific issue then please either sleep a night over it or ask someone else to answer it. Thank you.

jancborchardt commented 9 years ago

At the same time I can understand @Raydiation’s annoyment when the feature just doesn’t fully work as expected.

@clementhk @colmoneill I would suggest that we try to find a proper implementation for pull to refresh for the Mail app. Then we can experiment, and maybe we find something that the News app can use. :) https://github.com/owncloud/mail/issues/104

BernhardPosselt commented 9 years ago

@colmoneill @clementhk sry that it derailed that much, I'm pretty exhausted due to master thesis atm and had a hangover. I will try to improve the process ;)

@colmoneill as for opening issues: Unless the issue and solution is crystal clear, it is better to try discuss this in a cooperative and constructive manner. Issues that simply contain "it does not feel integrated" and a picture don't help at all and give off a picture of nitpicking and nagging. My first reaction to that was "ok so you dont like it, thank you". Also feedback sandwich.

@clementhk locking the issue was not meant as a personal offense, I wanted you guys to open a new issue with clear goals and problem statements since I felt it went in the wrong direction. Organizational reasons mostly so other people can dig into the issue without reading tons of conversation. You just slipped into a discussion which went downhill.

cosenal commented 9 years ago

@Raydiation: c'mon, locking the issue! dude, you gotta learn some damn politics! :p Except locking the issue, I don't see this violation of code of conduct.

About closing the issue too early: I used to dislike Raydiation's policy to close issues too instinctively, but I have to say, if that means 640 closed issues vs. 14 open issues and a very healthy bug tracker, then its policy is very welcome.

About the pull-to-refresh, I don't like the current solution myself (I expressed specific complains about the »Refresh« button in #770), but we weren't satisfied with the old solution either. A constructive proposal is very welcome, but until that comes I vote for keeping this issue closed.

@jancborchardt the pull-to-refresh has been gone for a while now, does it mean you don't dogfood?! :spank:

jancborchardt commented 9 years ago

About closing the issue too early: I used to dislike Raydiation's policy to close issues too instinctively, but I have to say, if that means 640 closed issues vs. 14 open issues and a very healthy bug tracker, then its policy is very welcome.

Come on – that’s a strawman argument and you know it. The amount of issues does not reflect the health of the project at all. I would even say trying to focus on keeping the issue count low results in a worse project quality. Software will always have bugs or feature requests and people have to accept that. There being only 14 issues in News is just not possible.

I even talked to @Raydiation about it that it’s strange that the News app doesn’t have more regular contributors. With the Mail app we already have a bunch of regulars even though we are still pre-0.1 and not even in the appstore yet. That’s because we are always friendly and inclusive and build a community by mentioning other people in PR reviews to check them out etc.

cosenal commented 9 years ago

You're right that we should have more regular contributors, and I don't think we don't because we're harsh with issue reports. Anyway, since we don't have many contributors, I find it useful that we can keep the bug tracker under control. We are both busy and we would feel lost if we had to handle hundreds of open issues.

If you browse through the 640 closed issues, you find out that almost most of them weren't reopened, because the OP understood the issue was resolved, or that they weren't constructive complains.

kklem0 commented 9 years ago

Hi guys,

I don't have a problem of issues being close as won't fix, my original concern was we should never lock an ongoing discussion, that is really bad. We cannot just open another issue with another suggestion, and hope that you will like it. We need to discuss it through so we understand each other, so we could open a proper suggestion. And IMHO, when you deny a proposal, it's a perfect place to keep the discussion going in the same place so we could come out with a better suggestion. If you know a more proper place, send the link or something. Asking people to go search around does not sound appropriate to me, there could be 20 things I'll find, do I just pick one and again hope that I would get yelled at? So again, my concern is not about closing an issue, but about stopping an ongoing discussion, which is the whole point of being a community. Thank you for unlocking this issue, I hope this explains my thought.

jancborchardt commented 9 years ago

@clementhk well said. :)

colmoneill commented 9 years ago

Hey @Raydiation sorry if this came off wrong. I recognize that a more detailed issue would have helped this get expressed properly. You'll have probably worked this out, but @jancborchardt and I had a hands on review of UX functions in different apps, and I grabbed many screenshots, reported many issues, and should have given more context to the rest of the contributors, sorry about that.

Regarding your way of dealing with issues, I believe this is entirely up to you, as you are the maintainer, plus, as is the case with this issue, it was not very well explained.

I was not aware of the keyboard navigation ability, I personally like this, although I agree with @jancborchardt that it is relatively advanced. Might I suggest to solve this by making this navigation more obvious? If refresh is a button, and not a scroll action, could the rest of the navigation be made obvious somewhere? Trying to be constructive with regards to the decision to not use 'scroll to refresh'.

BernhardPosselt commented 9 years ago

@colmoneill I was just really, really stressed out that weekend, looking back my reaction seems off ;) If you check the rest of the issues, I'm not sure if I've ever locked one issue, so this is more of a one time thing.

The question is: What is the real issue? Does the button itself look ugly aka purely cosmetic? Could it be solved by just changing the colors to the ones in http://apeatling.com/demos/web-ptr/ ? Just remove the button and display three dots like in the example? The button was added to basically hint users "hey if you go up you refresh" and to let mouse users refresh. Or is the problem that it should not be done by using scroll but pan instead? As for discoverability: mouse users could always just hit the feed again to reload.

As for touchscreen users, lets first leave that one out since the mobile interface is not a priority, we can talk about this later on. Something like hammerjs is the obvious solution here mentioned by @clementhk which can be tackled once https://github.com/owncloud/core/issues/11193 is resolved.