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

Fix some things for extract component #11137

Merged
merged 4 commits into from
Nov 3, 2024

Conversation

ryzngard
Copy link
Contributor

@ryzngard ryzngard commented Nov 1, 2024

Extract component had some odd behavior around MarkupTextLiteralSyntax selection.

Previously it would not allow a user to have a text literal as the start or end of the selection, it would always try to get the parent block. That works as intended for most cases where a user is highlighting inside a tag and the tag shouldn't be split. However, if the text is top level in the document it won't have a parent tag and thus would not have been considered a valid selection. This allows for that scenario.

There was also an issue if the selection was following a tag with a sibling text literal. If the selection ended after the brace but the text literal was not empty the selection would fail. For example:

[|<h1>Hello</h1>|]

Welcome to your new app

In the above the end of the selection is in the MarkupTextLiteralSyntax when it should be on the h1 node. A check for if the position was at the beginning of a text literal and it has a previous sibling was added to account for this.

@ryzngard ryzngard requested a review from a team as a code owner November 1, 2024 21:19
Copy link
Contributor

@davidwengier davidwengier left a comment

Choose a reason for hiding this comment

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

I think the logic in general here is starting to get convoluted. I don't think thats necessarily wrong, as I expect there to be edge cases and weirdness, and if we have decent tests (which we pretty much do) then I'm okay with it, but I think it wouldn't be bad to overcompensate this code with comments.

@ryzngard ryzngard requested a review from davidwengier November 2, 2024 00:32
@ryzngard
Copy link
Contributor Author

ryzngard commented Nov 2, 2024

@davidwengier I tried to add more comments, clean up the logic, and added a few more test for cases. Enough has changed it's probably worth a re-review

Copy link
Contributor

@davidwengier davidwengier left a comment

Choose a reason for hiding this comment

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

Love your work

@ryzngard ryzngard enabled auto-merge (squash) November 3, 2024 00:15
@ryzngard ryzngard merged commit 123b17b into dotnet:main Nov 3, 2024
11 of 12 checks passed
@dotnet-policy-service dotnet-policy-service bot added this to the Next milestone Nov 3, 2024
@ryzngard ryzngard deleted the extract_component_fixes branch November 4, 2024 02:38
@jjonescz jjonescz modified the milestones: Next, 17.13 P2 Nov 25, 2024
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.

3 participants