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

"Remove Link" option in the right-click context menu for links #5126

Merged
merged 17 commits into from
Jun 8, 2024

Conversation

NMazzy
Copy link
Contributor

@NMazzy NMazzy commented Apr 30, 2024

Description

#5046 Adds a "Remove Link" option in the right-click context menu for links, which when clicked would remove the URL and associated markup from the markdown text, leaving only the text.

Changes

  • Added a "menu.remove_link" field in the link right click context menu
  • Created remove-markdown-link.ts file in markdown-editor/util

Additional information

Tested on:

  • macOS Sonoma 14.1
  • Ubuntu 22.04

Copy link

boring-cyborg bot commented Apr 30, 2024

Thank you for opening your first PR! 🎉 We are very happy and would like to thank you very much for your contribution. If everything checks out, we'll make sure to review the PR as soon as possible and give feedback. In the meantime, to make the reviewing process as fast as possible, you can help us by checking the following things:

  • Did you follow the JSStandard coding style? - Did you comment everywhere where the necessity of a piece of code or the
    way it was implemented is not immediately obvious?
  • Did you attempt to stick as much to current coding habits as possible?
    (Note that this does not apply to pieces of code where we ourselves
    obviously violated good coding practices, which unfortunately happens
    sometimes. But please indicate this in your PR so that we know what you
    rectified!)

Furthermore, make sure that the linter does not complain, which will check your code on every new commit. If the linter task fails, make sure to run yarn lint locally and check the file eslint_report.htm which will tell you precisely what went wrong.
Stay sharp, and thanks again!

@NMazzy NMazzy marked this pull request as ready for review April 30, 2024 04:06
@nathanlesage
Copy link
Member

Thank you! Looks good so far -- just one comment: instead of slicing the entire node, you can also just search for the various child nodes that it should have. If I remember correctly, the link title that you want is not represented by its own node, instead you would have to check for two CodeMark nodes, and slice from the end of the first to the start of the second. That makes it more resilient to edge cases and leverages the parser more. I think I already implemented that snippet in the markdown AST parser module. I think searching for case 'Link' might turn it up

- Deal with either of the link nodes being selected (Text or Url)
- Use same logic as AST Parser to parse the LinkText
@MWBruce
Copy link
Contributor

MWBruce commented May 1, 2024

This has been implemented as suggested.

@nathanlesage
Copy link
Member

Looks good. I found one issue while testing this out, though: It doesn't work with angle brackets links, i.e.:

This is some text with a plain link: <https://www.google.com>

Right-clicking that link will give you the option to remove a link, but it won't actually remove the angled brackets as I would expect it.

@NMazzy
Copy link
Contributor Author

NMazzy commented May 1, 2024

Looks good. I found one issue while testing this out, though: It doesn't work with angle brackets links, i.e.:

This is some text with a plain link: <https://www.google.com>

Right-clicking that link will give you the option to remove a link, but it won't actually remove the angled brackets as I would expect it.

I didn't even know this was a possible markdown link :)
This latest commit handles this case now.

Copy link
Member

@nathanlesage nathanlesage left a comment

Choose a reason for hiding this comment

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

We're getting there! Just a few more things!

@NMazzy
Copy link
Contributor Author

NMazzy commented May 6, 2024

Hi @nathanlesage, I believe all your comments above, except the one regarding the reference style links have been implemented.

@MWBruce
Copy link
Contributor

MWBruce commented May 10, 2024

@NMazzy I think it might be @kyaso that is reviewing PR's at the moment.

@NMazzy NMazzy requested a review from nathanlesage May 14, 2024 23:45
@MWBruce
Copy link
Contributor

MWBruce commented May 22, 2024

@kyaso Further to a discord conversation I had with Nathan could you please advise if there is anything further required.

Copy link
Collaborator

@kyaso kyaso left a comment

Choose a reason for hiding this comment

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

Sorry for the late reply!

I just tested your PR locally, and it seems to do its job, so everything fine code-wise from my side.

One thing we should add is a Changelog entry (see https://github.com/Zettlr/Zettlr/blob/develop/CHANGELOG.md). You can add it under the "GUI and Functionality" section.

Once this has been done, I guess, @nathanlesage, we can merge?

@NMazzy
Copy link
Contributor Author

NMazzy commented Jun 5, 2024

Sorry for the late reply!

I just tested your PR locally, and it seems to do its job, so everything fine code-wise from my side.

One thing we should add is a Changelog entry (see https://github.com/Zettlr/Zettlr/blob/develop/CHANGELOG.md). You can add it under the "GUI and Functionality" section.

Once this has been done, I guess, @nathanlesage, we can merge?

Hi, a changelog entry has now been added under the "GUI and Functionality" section.

@nathanlesage
Copy link
Member

Looks good for me, too, but the linter is complaining about an extra semicolon and about another issue (see the unit test) — once that's fixed we can merge. Thanks @kyaso for the code review!

@NMazzy
Copy link
Contributor Author

NMazzy commented Jun 8, 2024

Looks good for me, too, but the linter is complaining about an extra semicolon and about another issue (see the unit test) — once that's fixed we can merge. Thanks @kyaso for the code review!

Apologies should have checked this before hand, all should be fixed now.

@nathanlesage
Copy link
Member

All good, no worries! I'll wait for the unit tests to finish and then merge it in! Thanks again for the work!

@nathanlesage nathanlesage merged commit 21e3e7e into Zettlr:develop Jun 8, 2024
1 check passed
Copy link

boring-cyborg bot commented Jun 8, 2024

There we go, your PR got merged! Welcome to the party! 🔥

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.

None yet

4 participants