-
Notifications
You must be signed in to change notification settings - Fork 250
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
[SuperEditor] Limit tag expansion to caret position (Resolves #2240, #2241, #2242) #2387
Merged
Merged
Changes from 2 commits
Commits
Show all changes
7 commits
Select commit
Hold shift + click to select a range
eacfcf8
[SuperEditor] Limit tag expansion to caret position (Resolves #2240)
angelosilvestre 23d2527
Fix tag being re-applied after being canceled
angelosilvestre aea32be
PR updates
angelosilvestre f9e5819
PR updates
angelosilvestre bacfd11
Extract findTagAroundPosition
angelosilvestre 07e0e21
PR updates
angelosilvestre d9278a2
PR updates
angelosilvestre File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
I don't understand this description of
endPosition
. This method was intended to only look for tags that touchexpansionPosition
, which is a singular position. There shouldn't be a range involved. So the idea of anendPosition
doesn't make sense to me.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.
So should we keep this method as it was before and modify the reaction to adjust the reported tag?
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.
My point was that I don't know what you're trying to do here because it doesn't seem to make sense. I can't answer that question until I first understand what exactly you were trying to do.
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.
What I'm trying to do is to prevent the tag to expand beyond the caret position. For example, if we have the string "helloworld" and the user types a "/" at the middle, changing it to "hello/world", the string "world" shouldn't be attributed as a tag. The user can input a tag in the middle of a word, but the tag shouldn't expand beyond the caret position.
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.
I understand the feature goal. What I don't understand is the code that I commented on. I don't understand how/why achieving the feature goal that you described requires a caller to pass some thing called an "end position". Can you explain what this approach is doing and why it makes sense for the feature goal?
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.
@angelosilvestre just need an update whether this is in progress or stagnant.
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.
Hi @alterhuman. I'm resuming the work on this PR.
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.
@alterhuman Can you try this PR again?
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.
@angelosilvestre tested everything. All working correctly like previous solution except the previous remaining issue:
Action tag dropdown popping up after reopening a document and focus the caret on the text containing the tag rule /, after the slash. To reproduce:
Instead of having an empty document add this text:
Focus caret at n, the dropdown pops up.
I have already filed a separate issue for this #2392
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.
@matthew-carroll been waiting for this PR to merge since a long time, can you please review this?