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: remove internal link type #11553

Merged
merged 2 commits into from
Sep 17, 2024
Merged

Conversation

JammingBen
Copy link
Collaborator

@JammingBen JammingBen commented Sep 9, 2024

Description

Removes the internal link type since the permanent link makes it obsolete. Existing internal links still resolve correctly to ensure backwards compatibility.

Also removes the action in favor of the new action for copying permanent links.

Related Issue

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Technical debt
  • Tests
  • Documentation
  • Maintenance (e.g. dependency updates or tooling)

@JammingBen JammingBen self-assigned this Sep 9, 2024
@JammingBen JammingBen force-pushed the feat/remove-internal-link-role branch 3 times, most recently from 779708f to b1f043f Compare September 9, 2024 15:04
@JammingBen
Copy link
Collaborator Author

@tbsbdr @kulmann There is one edge case when the user doesn't have the permission to create public links. In the past, creating internal links was still possible. With this change though, those users can't create links at all, which is a bit tricky because of the "Copy link" quick action. We can easily check if the resource as a link share and then decide whether to display the quick action or not. But we can't check if the resource has a quick link. And since the quick action is designed to only copy quick links, this results in an issue when a resource has a normal link, but not a quick link.

We could change to quick action to factor in regular links as well. Or simply ignore this edge case.

@tbsbdr
Copy link
Contributor

tbsbdr commented Sep 10, 2024

I'd opt to make the permalink the default quickaction link. we already talked about it some time ago. what do you think @kulmann ?

@kulmann
Copy link
Member

kulmann commented Sep 10, 2024

I'd opt to make the permalink the default quickaction link. we already talked about it some time ago. what do you think @kulmann ?

default was in always use the permalink?!

If I remember correctly I advocated to kill the quick link concept entirely and instead just use any first link that can be found on the resource. If there is none -> permalink.

Since this PR is about removing the internal link type, a good step in between is to copy the permalink when the user isn't allowed to create a public link. Not so sure about always copying the permalink as the only option for the quick action. 🤔

@tbsbdr
Copy link
Contributor

tbsbdr commented Sep 10, 2024

Lets always use the perma link in the quickaction. ok @JammingBen @AlexAndBear @kulmann ?

@JammingBen
Copy link
Collaborator Author

Lets always use the perma link in the quickaction. ok @JammingBen @AlexAndBear @kulmann ?

What about the action in the right-click context menu? We have the same problem here. Should it also just copy the permanent link?

Removes the internal link type since the permanent link makes it obsolete. Existing internal links still resolve correctly to ensure backwards compatibility.
@kulmann
Copy link
Member

kulmann commented Sep 11, 2024

Lets always use the perma link in the quickaction. ok @JammingBen @AlexAndBear @kulmann ?

What about the action in the right-click context menu? We have the same problem here. Should it also just copy the permanent link?

IMO yes. Maybe it makes sense to rename the Share menu item to Share or link or something like that, because everything is in the same panel anyway.

@JammingBen
Copy link
Collaborator Author

Okay, so to get this straight: The only way to create a public link will be via the sidebar in the future - no more (quick-) action for that, right? I like that, in fact that's what I was suggesting a few week ago. I just want to make sure we are all on the same page here, because I can certainly imagine that some users won't like it.

@kulmann
Copy link
Member

kulmann commented Sep 11, 2024

Okay, so to get this straight: The only way to create a public link will be via the sidebar in the future - no more (quick-) action for that, right? I like that, in fact that's what I was suggesting a few week ago. I just want to make sure we are all on the same page here, because I can certainly imagine that some users won't like it.

Yep, discussed with Tobi yesterday, that we want to try exactly what you're describing. 👍

Removes the action in favour of the new action for copying permanent links.
Copy link

sonarcloud bot commented Sep 11, 2024

@JammingBen JammingBen marked this pull request as ready for review September 11, 2024 08:37
Copy link
Contributor

@AlexAndBear AlexAndBear left a comment

Choose a reason for hiding this comment

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

Absolutely stunning 🤩

@JammingBen JammingBen merged commit b37ed70 into master Sep 17, 2024
3 checks passed
@JammingBen JammingBen deleted the feat/remove-internal-link-role branch September 17, 2024 05:21
@micbar micbar mentioned this pull request Oct 1, 2024
18 tasks
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.

Remove internal link
4 participants