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: Unindent empty list markup on Enter #2772

Merged
merged 2 commits into from
Jun 4, 2020

Conversation

sinkuu
Copy link
Contributor

@sinkuu sinkuu commented Mar 15, 2020

Fixes #2614

cc @Rishgod

@tessus
Copy link
Collaborator

tessus commented Mar 15, 2020

The wrong issue is linked.

Copy link
Collaborator

@tessus tessus left a comment

Choose a reason for hiding this comment

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

It works nicely with lists, but not with checkboxes. Any chance you could add that as well?

@sinkuu
Copy link
Contributor Author

sinkuu commented Mar 16, 2020

Added.

Copy link
Collaborator

@tessus tessus left a comment

Choose a reason for hiding this comment

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

Perfect, thanks.

One last thing: When indented and hitting enter, the cursor should reset to column position 1. e.g.:

Current behavior:

- item
    - item
    - item (hit enter in this line)
    x   <---- cursor is here

But it should rather be like this:

- item
    - item
    - item (hit enter in this line)
x   <---- cursor is here

@sinkuu
Copy link
Contributor Author

sinkuu commented Mar 16, 2020

Another possibility is to unindent, which is adopted by Dropbox Paper and Evernote.

- item
    - item
    - item
- x  <---- cursor is here (hitting enter again removes `- `)

EDIT: As a reference point related to Joplin, TinyMCE uses this behavior too.

@tessus
Copy link
Collaborator

tessus commented Mar 16, 2020

Another possibility is to unindent

Yes, this would also be a nice solution, but probably more complicated to implement.
Your latest fix was already great, so the PR looks ready to me.

But maybe Laurent does like your idea better, since people coming from Evernote certainly would feel right at home with that behavior. Let's wait for Laurent's input.

@tessus tessus added the desktop All desktop platforms label Mar 16, 2020
@miciasto
Copy link
Contributor

miciasto commented Mar 17, 2020

Hi @sinkuu thanks for the changes. I deleted my comments immediately and was going to try again later, but it seems you saw them anyway!

@miciasto
Copy link
Contributor

Another possibility is to unindent

and then clear after that.

@tessus
Copy link
Collaborator

tessus commented Mar 17, 2020

@mic704b

and then clear after that.

I think in this case only after a second Enter, because you might want to continue the list....

@sinkuu
Copy link
Contributor Author

sinkuu commented Mar 17, 2020

Unindent on Enter (3204eb0):

Peek 2020-03-17 18-13

Clear indentation on Enter (ce6355e):

Peek 2020-03-17 18-18

@miciasto
Copy link
Contributor

Looks good.

Does this have any effect when entering numbered lists? Have you tested mid-line multiline selections?

It's a shame we have no way to automated test this, to make sure no one breaks it in the future.

@sinkuu
Copy link
Contributor Author

sinkuu commented Mar 18, 2020

Does this have any effect when entering numbered lists?

It works the same for all Markdown list syntaxes (-, *, 1. etc.).

Have you tested mid-line multiline selections?

I added multiSelectAction: 'forEach':
Peek 2020-03-18 10-03

@sinkuu
Copy link
Contributor Author

sinkuu commented Mar 18, 2020

@mic704b I made automated tests here: https://github.com/sinkuu/joplin-ace-indent-test/blob/master/spec/indentSpec.js I don't know how I can integrate this to Joplin (if that's a good idea) though. The tests require Ace's MockRenderer which is not included in published packages.

@tessus
Copy link
Collaborator

tessus commented Mar 19, 2020

I think the behavior should be like the way you have alerady described:

- item
    - item
    - item
- x  <---- cursor is here (hitting enter again removes `- `)

This behavior makes the most sense. Then I think we are good to merge. Just waiting for Laurent's input.

@sinkuu
Copy link
Contributor Author

sinkuu commented Mar 19, 2020

Changed to unindenting behavior.

@tessus
Copy link
Collaborator

tessus commented Mar 20, 2020

Great. This helps a lot with usability.

@miciasto
Copy link
Contributor

I made automated tests here

@sinkuu that looks really good. Nice work.

I don't know how I can integrate this to Joplin (if that's a good idea) though

I think when it is testing behaviour implemented in Joplin then it makes a lot of sense. (Though it might even make sense when testing required behaviour of dependencies too). However let's see what Laurent thinks, when things calm down a bit.

@laurent22
Copy link
Owner

The test units you've created look good @sinkuu. Any chance you could add them to this pull request? You could put the test in CliClient/tests and feel free to add any required dependency to CliClient/package.json as dev dependency.

@sinkuu
Copy link
Contributor Author

sinkuu commented Apr 10, 2020

Done.

@PackElend
Copy link
Collaborator

ready to merge?

@sinkuu sinkuu changed the title Desktop: Delete a list markup from an empty list item on hitting Enter Desktop: Unindent empty list markup on Enter Apr 17, 2020
@laurent22 laurent22 added the high High priority issues label Apr 30, 2020
@laurent22
Copy link
Owner

Sorry for being late to review this @sinkuu, and unfortunately I see there are some conflicts, due to the recent refactoring of NoteText.jsx, please could you take a look at them?

I've moved the previous code from your pull request to a custom hook in AceEditor/utils/useListIdent.ts. I believe you might need to move some of your changes to this file, and the rest probably in AceEditor.tsx.

@tessus
Copy link
Collaborator

tessus commented May 22, 2020

@sinkuu have you had the time to look into this? I was quite busy and unavailable the last 2 weeks, so I couldn't look into it myself.

@tessus
Copy link
Collaborator

tessus commented May 31, 2020

I really would hate seeing this not merged. This made working with lists and checklists a lot easier and better.

@tessus
Copy link
Collaborator

tessus commented Jun 4, 2020

Works perfectly! Thanks!

@tessus
Copy link
Collaborator

tessus commented Jun 4, 2020

@laurent22 can I merge this?

@laurent22
Copy link
Owner

Yes, if it's working for you then let's merge. Thanks @sinkuu!

@laurent22 laurent22 merged commit 949c92f into laurent22:master Jun 4, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
desktop All desktop platforms high High priority issues
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Uneven Behavior of List Elements
5 participants