-
-
Notifications
You must be signed in to change notification settings - Fork 835
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
Improve selected text stylization #2961
Conversation
let text = textarea.value.slice(textarea.selectionStart, textarea.selectionEnd); | ||
let selectionStart = textarea.selectionStart; | ||
let selectionEnd = textarea.selectionEnd; | ||
const lines = text.split('\n'); | ||
const undoStyle = lines.every((line) => line.startsWith(prefix) && line.endsWith(suffix)); | ||
let prefixToUse = blockPrefix.length > 0 ? blockPrefix : prefix; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe a falsy vs nonfalsy check would be more appropriate here? Also, shouldn't this be based on the number of lines selected?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's under the multilineStyle function. So this part of code will affect multiple selections only.
I can't see the advantage of using falsy check here? What could possibly go wrong when we keep it this way?
@askvortsov1 @luceos I couldn't find time to review change request due to my job, huge sorry for that. I see that you've merged the PR from the markdown extension (and tagged with 1.1?) but this one is still open. As you're preparing to drop a new release, I'm just here to remind you that this PR must be merged in order to make the spoiler button work properly. |
I tested it out, currently it works, but just uses many line spoilers instead of a single block spoiler. This isn't ideal, but it's also not entirely broken, so since we're practically released, we'll push this improvement to the next version. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested locally, LGTM
Changes proposed in this pull request:
It was using inline affix if you have block prefix but don't have block suffix.
This makes impossible to use suffix-less block syntaxes (e.g. spoiler).
It makes sense to use block affix (if there are any) for multiline selection.
It makes sense to surround multiline selection with new lines if you have block prefix but don't have block suffix.
I needed to set
surroundWithNewlines
argument totrue
to achieve this behaviour on the block spoiler. But this also affected the inline spoiler and that's undesired. So I assumed that If you don't have any suffix and try to style multiple lines at once, then you probably will want to surround selection with new lines.Confirmed
Required for: