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

Alter suggested bookmarklet to use Markdown #5384

Closed
wants to merge 3 commits into from

Conversation

JesseWeinstein
Copy link
Contributor

Switch it to use the content parameter, which doesn't alter the results, and insert some basic Markdown (header for the title, block quote for the selected text (still need to handle the case of no-selection better)) and put the URL at the end.

@Raven24
Copy link
Member

Raven24 commented Nov 10, 2014

sorry, I always found that line very hard to read...
could you post a before-after comparison of the url it generates, please.

@JesseWeinstein
Copy link
Contributor Author

Certainly. What I'll do is make a new set of commits, that first breaks up the bookmark into multiple lines, then changes it.

To improve the readabilit of diffs that modify it, per diaspora#5384 (comment)
Switch to using content parameter, which doesn't alter its value, unlike the other ones.

Use a H2 header for the document.title, quote the selected text, and put the URL at the bottom, in a separate paragraph.
Intentionally do a converting-comparison against the empty string, to see if text is selected.
@JesseWeinstein
Copy link
Contributor Author

OK, I've re-done the commit, off current develop, first by dividing the existing literal into separate lines, then making my changes (which I also broke into two commits, for readability). I look forward to your review!

@svbergerem
Copy link
Member

Tested this with iceweasel 33.1:

selected text:

no selected text:

@JesseWeinstein
Copy link
Contributor Author

Hm, it looks like the newlines didn't take, for some reason. I'll test further.

@jhass
Copy link
Member

jhass commented Feb 28, 2015

@JesseWeinstein Hey, still want to look into it or shall we put this up for adoption?

@JesseWeinstein
Copy link
Contributor Author

I'm still interested, but I'd be delighted if someone with more knowledge of the codebase could look into it too.

@Raven24
Copy link
Member

Raven24 commented May 1, 2015

superseded by #5904
thanks for starting the initiative on this!

@Raven24 Raven24 closed this May 1, 2015
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.

4 participants