Skip to content

Fix: Pasting of html with similar matches that have links #88

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

Merged
merged 3 commits into from
Feb 19, 2024

Conversation

susanwere
Copy link
Contributor

@susanwere susanwere commented Jul 31, 2023

Hi 👋🏼, this is an attempt to fix the issue below where Pasting HTML with links can place the link in the wrong place

The original fix was submitted here. This PR attempts to look at a few edge cases that came up as shown indicated in the issue above. These include:

foo bar baz [bar](https://www.abcxyz.com)

Which would result to the following after pasting

foo [bar](https://www.abcxyz.com/) baz bar

and

foo bar 
bar baz [bar](https://www.abcxyz.org/) 
 
baz [baz](https://www.abcxyz.com/) foo

Which would result to the following after pasting

foo bar
[bar](https://www.abcxyz.org/) [baz](https://www.abcxyz.com/) bar

baz baz foo

@susanwere susanwere changed the title Fix: Pasting of html with similar labels that have links Fix: Pasting of html with similar matches that have links Jul 31, 2023
@susanwere susanwere marked this pull request as ready for review July 31, 2023 14:12
@susanwere susanwere requested a review from a team as a code owner July 31, 2023 14:12
@primer-css
Copy link

👋 Hello and thanks for pinging us! This issue or PR has been added to our inbox and a Primer first responder will review it soon.

  • 🎨 If this is a PR that includes a visual change, please make sure to add screenshots in the description or deploy this code to a lab machine with instructions for how to test.
  • If this is a PR that includes changes to an interaction, please include a video recording in the description.
  • ⚠️ If this is urgent, please visit us in #primer on Slack and tag the first responders listed in the channel topic.

Copy link
Contributor

@keithamus keithamus left a comment

Choose a reason for hiding this comment

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

This looks like a great change, and thanks very much for adding tests!

I think this might be a good opportunity to look at our current tests and add a lot more robustness to them. It's non-obvious from the outset as to why this change causes such as behavioural change, and looking at the existing test suite, I'm not filled with confidence that this wouldn't introduce a regression somewhere (especially around dropping the else clause).

I'm happy for this PR to be merged and shipped but if you have some extra time I think it could be great to add some additional tests of various input where there may be edge cases.

@susanwere
Copy link
Contributor Author

This looks like a great change, and thanks very much for adding tests!

I think this might be a good opportunity to look at our current tests and add a lot more robustness to them. It's non-obvious from the outset as to why this change causes such as behavioural change, and looking at the existing test suite, I'm not filled with confidence that this wouldn't introduce a regression somewhere (especially around dropping the else clause).

I'm happy for this PR to be merged and shipped but if you have some extra time I think it could be great to add some additional tests of various input where there may be edge cases.

I have added a few more tests related to links. I think we can keep an eye on the changes and see if there is more regression....is there a way to test before shipping?

@keithamus
Copy link
Contributor

Thanks for adding these!

We could make this a mode (i.e. add an option to enable the code path) and ship it as a feature flag in dotcom, but I don't think that'll be necessary. If it regresses we can revert and add more testing.

@keithamus keithamus merged commit ca6eeb0 into github:main Feb 19, 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