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

Desktop: Enhance notelist focus behaviour #2520

Merged
merged 7 commits into from
Mar 11, 2020

Conversation

miciasto
Copy link
Contributor

@miciasto miciasto commented Feb 18, 2020

This PR contains two enhancements to focus behaviour in the note list:

Focus following delete

  1. Delete a note from the note list (eg using Delete key or context-menu)
  2. Use Up or Down key to select the next note

Expected: the next note is selected, the user can continue navigating with the keys
Actual: nothing, the note list does not respond to the navigation keys anymore

Focus when navigating "away" from multiple-note selection.

  1. Select multiple notes in the note list (eg Ctrl-A, or using mouse)
  2. Use Up or Down key to select the next note

Expected: the next note is selected, and the previous notes are deselected
Actual: nothing, the note list does not respond to navigation keys anymore

This fix also fixes another issue discussed in forum as a side effect.

@tessus tessus added the desktop All desktop platforms label Feb 20, 2020
@laurent22
Copy link
Owner

Just need to fix the conflict and we can merge. (The "app" folder was removed so you will need to move your file one level down)

@miciasto
Copy link
Contributor Author

I'm going to postpone this for now, until I get a chance to look at it.

It works for note delete, but breaks new note behaviour.

@miciasto miciasto closed this Feb 22, 2020
@miciasto miciasto reopened this Mar 2, 2020
@miciasto
Copy link
Contributor Author

miciasto commented Mar 2, 2020

Fixed and ready for review.

@tessus
Copy link
Collaborator

tessus commented Mar 11, 2020

I'd like to merge this. Is there anything left to do? @laurent22 you already gave the ok to merge, but there were a few changes after your comment. Please advise.

@laurent22
Copy link
Owner

Looks good but I didn't test, however if you guys think it's ready to merge, please go ahead.

@tessus
Copy link
Collaborator

tessus commented Mar 11, 2020

Oh, yes, I forgot to add the PR-tested label. I actually did test this.

@tessus tessus merged commit 34a1c96 into laurent22:master Mar 11, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
desktop All desktop platforms
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants