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

feat(supersearch): Insert space before qualifier (LWS-286) #1198

Open
wants to merge 5 commits into
base: develop
Choose a base branch
from

Conversation

jesperengstrom
Copy link
Contributor

@jesperengstrom jesperengstrom commented Jan 8, 2025

Description

Tickets involved

LWS-286

Solves

Avoids breaking a qualifier pill by typing stuff before it, inadvertently altering the qualifierKey.

Summary of changes

  • Provide a insertSpaceBeforeQualifier function with the plugin - a transaction filter that checks if any atomic range overlaps the cursor position. If so, add a blank space at the position (before the pill).
  • For easier access to the atomic ranges, we now build a separate range set while creating the widgets by using a RangeSetBuilder. This can then be passed directly to Editorview.atomicRanges and the insertSpace functions, and previous workarounds (atomic prop, filterAtomic function are no longer needed).
  • Some general refactoring of the plugin.

@jesperengstrom jesperengstrom changed the title fix(supersearch): Insert space before qualifier (LWS-286) feat(supersearch): Insert space before qualifier (LWS-286) Jan 8, 2025
@jesperengstrom jesperengstrom marked this pull request as ready for review January 8, 2025 16:47
Copy link
Contributor

@johanbissemattsson johanbissemattsson left a comment

Choose a reason for hiding this comment

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

Great work (also with the usage of RangeSetBuilder)! 🙌

See comments regarding the number of spaces added.

changes: {
from: newCursorPos,
to: newCursorPos,
insert: ' '
Copy link
Contributor

Choose a reason for hiding this comment

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

I get the feeling that one space to many is added when pressing space before a valid qualifier (when there should only be one?).

Changing line 18 to insert: '' and line 21 to selection: { anchor: oldCursorPos } makes it IMO feel more right – or what do you think? Hopefully it doesn't breaking something else...

Copy link
Contributor

Choose a reason for hiding this comment

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

Edited the comment above now as I accidentally added a whitespace in-between the quotes (it should be insert: '' instead of insert: ' ')

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm removing the changes property altogether maybe makes more sense? It seems to have the same effect:

const insertSpaceBeforeQualifier = (getRanges: () => RangeSet<RangeValue>) => {
	return EditorState.transactionFilter.of((tr) => {
		if (!tr.docChanged || (tr.isUserEvent('delete') && tr.state.selection.main.head === 0)) {
			return tr;
		} else {
			let insert = {};
			const atomicRanges = getRanges();
			const oldCursorPos = tr.startState.selection.main.head;

			atomicRanges.between(oldCursorPos, oldCursorPos, () => {
				insert = {
					sequential: true,
					selection: { anchor: oldCursorPos}
				};
				return false;
			});
			return [tr, insert];
		}
	});
};

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants