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 permalink feature to include the full URL #56

Open
waldyrious opened this issue Nov 13, 2017 · 4 comments
Open

Fix permalink feature to include the full URL #56

waldyrious opened this issue Nov 13, 2017 · 4 comments
Assignees
Labels

Comments

@waldyrious
Copy link
Owner

waldyrious commented Nov 13, 2017

The "copy permalink" feature was implemented in #42, but there seems to be an issue. Relevant quotes from #41:

if I search for a string (or load a random article) and then click the clipboard icon, what's copied is not the entire URL but just, e.g. /primerpedia/?search=John_Doe.

and

This [happens] since the function getShareableLink only uses location.pathname which of course only contains /primerpedia.
It's a straight forward fix to add location.origin as well but I'm fairly certain that I tested this functionality thoroughly.

Probably wanted to append it to

var shareLink = window.location.href;

but forgot?

and

While trying to implement this feature I found that it's not completely done with just adding window.location.origin (as it can start with // which would break the link). There needs some proper url handling (add if search is missing, change if it's available) in place.

@waldyrious waldyrious added the bug label Nov 13, 2017
@Jan-Ka Jan-Ka self-assigned this Nov 13, 2017
@Witia1
Copy link
Contributor

Witia1 commented May 19, 2019

In function renderSearchResult I replaced
copyShareLinkInputElement.value = getShareableLink(encodedArticleTitle);
to
copyShareLinkInputElement.value = shareLink;
It gets full url but previous page from history, not current.

@waldyrious
Copy link
Owner Author

waldyrious commented Jun 10, 2019

Thanks for the information, @Witia1. I actually am considering whether having a dedicated button to copy the URL in such a minimal interface is worth it, since the functionality is already accessible otherwise.

Specifically, the URL is already updated to match the current page being viewed (since #22), so the user can either copy the URL from the address bar, on a desktop browser, or share the page using the native sharing flow on mobile.

With all due respect and appreciation to the work that went into implementing this feature, given the above I am, at the moment, inclined to remove it altogether. What do you think?

@marado
Copy link

marado commented Nov 29, 2022

I think both options are acceptable, but I'd like to suggest that a decision should be made, because, as it is, we have the worst possible behavior.

@waldyrious
Copy link
Owner Author

Given the lack of response, and in the interests of simplifying the code to be more maintainable (without significant loss of functionality, as mentioned in my last comment), I'd say the best option is to remove the feature. Would you be willing to submit a patch, @marado?

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

No branches or pull requests

4 participants