search

guardian / scribe-plugin-noting

9 stars 3 forks source link

Can't place caret after note when noting out the end of a paragraph #85

Closed robinedman closed 9 years ago

robinedman commented 9 years ago

You can't place caret after the note when noting out the end of a paragraph. Also annoying if you note the end of a scribe instance, e.g at the end of an article, or before a picture, embed, pullquote etc.

Solve this by inserting a space after the note if and only if the note is at the end of a paragraph?

Get back to Katherine Le Ruez, who also reported this, when fixed.

cutandpastey commented 9 years ago

There is an explicit check in createNoteFromSelection, can we just remove this? Also, how do you test this?

robinedman commented 9 years ago

Idea for a test: Note some text at the end of a paragraph. Type a character. Check if the character is within the same paragraph?

You mean this line right https://github.com/guardian/scribe-plugin-noting/blob/7a3f6e8d0c78dbbce67fce36650f3df51a0ab97a/src/actions/noting/create-note-from-selection.js#L68?

That prevents a new paragraph from being created when you note out a whole paragraph. I think that condition will still need to be there. (Not sure whether we have a test for this atm.)

This issue is present for every paragraph so I think we might need another condition, or expand this one. Messy...

cutandpastey commented 9 years ago

Isn't https://github.com/guardian/scribe-plugin-noting/blob/7a3f6e8d0c78dbbce67fce36650f3df51a0ab97a/src/actions/noting/create-note-from-selection.js#L76 exactly what you want to do? If the selection ends at the end of a paragraph place the caret within the paragraph but after a space?

robinedman commented 9 years ago

This condition only checks if it's anywhere within a paragraph. But if we add another condition before that one that checks if it's within and at the end of that paragraph then absolutely!

I can imagine some people might get annoyed by this behaviour as well so let's check with @alastairjardine. Since Composer strips spaces before saving the content I think this'll probably be fine as a solution.

robinedman commented 9 years ago

I guess we don't want a space, but a zero width space. And this might very well be a bit fiddly (like the rest of this logic I guess).

cutandpastey commented 9 years ago

Closing as https://github.com/guardian/scribe-plugin-noting/pull/87 has been merged.