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: Start edit from top #2025

Closed

Conversation

daniellandau
Copy link
Contributor

This hopefully alleviates some of the issue in
#1838. At least for me I don't really
need a way to scroll quickly, but rather I usually want to add to the top.

This commit makes the mobile experience more consistent both with the desktop
experience and internally. In the desktop version notes are shown from the top,
not the bottom and on mobile the "view note" view already defaults to showing
the top of the note and in my opinion it's somewhat surprising when changing to
edit that it suddenly jumps to the end.

@tessus
Copy link
Collaborator

tessus commented Oct 26, 2019

IMO there should be a setting so that people can choose. But that's not my decision. Let's see what Laurent thinks of the change.

@tessus tessus added the mobile All mobile platforms label Oct 26, 2019
@tessus
Copy link
Collaborator

tessus commented Oct 26, 2019

Also, I think you did not install the pre-commit hook. The linter failed in the build step.

https://github.com/laurent22/joplin/blob/master/BUILD.md#building-the-tools

This hopefully alleviates some of the issue in
laurent22#1838. At least for me I don't really
need a way to scroll quickly, but rather I usually want to add to the top.

This commit makes the mobile experience more consistent both with the desktop
experience and internally. In the desktop version notes are shown from the top,
not the bottom and on mobile the "view note" view already defaults to showing
the top of the note and in my opinion it's somewhat surprising when changing to
edit that it suddenly jumps to the end.
@daniellandau
Copy link
Contributor Author

I indeed did not have the hooks installed, thanks for the pointer. Hopefully fixed now.

@laurent22
Copy link
Owner

laurent22 commented Oct 31, 2019

Thanks for the pull request, but I'm not sure we should merge it.

The way it is now is indeed not great, but with this change it's the same situation - for example if I scroll down to the bottom of a note and click Edit, I'm now back at the top. Ideally, when pressing edit, the cursor should go roughly where we are - for example if we are at 50% scroll, the cursor should go at 50% too. But I guess that's tricky to implement?

I'm not keen on an option for this either.

@daniellandau
Copy link
Contributor Author

As long as we can get the current scroll position (or more precisely the current top of text in view I guess) in a sensible way from the view, it shouldn't be all that tricky to start from that.

@Outi-s
Copy link

Outi-s commented Oct 31, 2019

A relative scroll to top or scroll to bottom button could be an option.

@laurent22
Copy link
Owner

I think the issue is that there's currently nothing to get the webview scroll position, it would have to be implemented first.

@axq
Copy link
Contributor

axq commented Nov 15, 2019

I understand Laurent's concerns, maybe a poll could help decide if merging an imperfect fix makes sense? As is, the combination of broken scrolling with jumping to end of note in edit makes it really hard to use on Android for those who add to notes from the top. But I don't know if those of us adding from the top are the overwhelming majority.

@daniellandau
Copy link
Contributor Author

I haven't had time to do anything yet, but it should be relatively to simple to implement a scroll listener inside the web view that posts the current scroll position back to React Native.

@daniellandau
Copy link
Contributor Author

daniellandau commented Nov 17, 2019

I pushed a draft of how it would work with monitoring the scroll position of the web view here: daniellandau@498e234.

Unfortunately that doesn't work without updating react-native to 0.60 because of this: facebook/react-native@84a1cac.

Updating to react-native 0.60 is something I don't want to include here because at least https://github.com/react-native-community/cli/blob/master/docs/autolinking.md and possible other issues with updating react-native that I don't feel qualified to address.

Is an update of react-native in the pipeline? If it's not an immediate priority, maybe this pull request could go in as is based on it still being better than the current situation even if not perfect?

@laurent22
Copy link
Owner

Thanks for looking into it @daniellandau. I indeed plan to upgrade RN as soon as possible, so that's not an issue.

Does your implementation work if you press on a link and jump to an anchor from within a note - i.e. is the 'scroll' event triggered in this case?

I saw you've added the code to weblib but note that this is shared by the mobile and desktop app. A better place might be in the JS injected in NoteBodyViewer, that way it will be only for mobile.

@laurent22
Copy link
Owner

Closing PR for now. Feel free to ask to reopen if you'd like to continue working on it.

@laurent22 laurent22 closed this Dec 28, 2019
@daniellandau
Copy link
Contributor Author

Scroll listeners normally do work when clicking on anchor links, so I don't see why they wouldn't. I'll continue working on this once the update to react-native is done.

@gitoss
Copy link

gitoss commented Jan 25, 2020

I opened another ticket and didn't see this closed one, sorry, my bad.

Please re-open and keep open this ticket (and freel free to close #2287) until it's fixed, this is the one major annoyance right now - in concuntion with slow scolling behavior and non-working sroll bar on mobile (Android).

This desereves a temp fix until a proper solition (start edit at current view position) is implemented. I've read that the @laurent22 isn't keen on an option, but this would be an easy way to help affected users right now, wouldn't it? And it can be removed easily later on.

  1. As view always starts at the top, and edit at the bottom this is completely counter-intuitive right now. Most people would click edit without scrolling down first, so there is a disruption in document perception.

  2. Personal usage background: With most of my notes, I'm used to put the most current information on top, pushing older things down. This way, I see the most current information right away on document open. => I've seen the idea for a poll, but me coming up with the same issue independently makes it +1 :-) and is probably an indication something should be done about it?

Thanks!

@h2oskierjb
Copy link

Cursor beginning to edit at the bottom of note instead of the top (as shown in view mode) is a real problem as I have a significant amount of info in notes and can't scroll up - and unfortunately makes the app in Android unusable. Any progress on this?

Repository owner deleted a comment from entodoays Dec 18, 2020
@altsalt
Copy link

altsalt commented Dec 23, 2020

@daniellandau it appears that RN was updated. Is this PR still viable and if so, would you mind bringing eyes back to it? Thanks for all of your work on this project @laurent22 and others!

@daniellandau
Copy link
Contributor Author

Yes, I noticed but haven't had time to look at it yet. It should be quite doable now.

@daykx
Copy link

daykx commented Jun 18, 2021

Latest changelog of version 2.0.2 in Google Play finally says:

Improved: Focus note editor where tapped instead of scrolling to the end

But my notes still starts edit at the end, or I get it wrong and it's different issue?

@gitoss
Copy link

gitoss commented Jul 1, 2021

@daykx nope, the annonced improvment seems to be only inside the Edit mode, not for switching from View to Edit mode.

There is a new bug ticket, after the older ones were closed: #5140

@daykx
Copy link

daykx commented Sep 30, 2021

It turns out that editing starts from top (though not where tapped) with new beta editor, there is switch for it in configuration

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
mobile All mobile platforms
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants