theCrag / website

theCrag.com: Add your voice and help guide the development of the world's largest collaborative rock climbing & bouldering platform
https://www.thecrag.com/
112 stars 8 forks source link

Allow embedding of photos / pictures in markdown #511

Closed brendanheywood closed 10 years ago

brendanheywood commented 12 years ago

Already talked about ages ago but it seems we didn't make an issue for it.

ie check out dwarf land:

DomL has linked directly to the photo which is cool but would be better to just embed the photo, and link to the photo summary page instead of the raw photo.

http://www.thecrag.com/area/11812045

the other example from ages ago:

http://www.thecrag.com/area/11872453

cgome commented 12 years ago

In mark up, would this be as simple as just adding a URL eg

http://www.mountain-forecast.com/locationmaps/Mount-Arapiles.10.jpg

and having it automatically ember the photo.

If it was going to be trickier than that- (eg requiring html tags, etc) then we'll lose a lot of people- myself included.

It would maybe require some sort of mark-up toolbar with an embed image option similar to what I'm used to using in tumblr, etc.

On Fri, Apr 20, 2012 at 2:53 PM, Brendan Heywood < reply@reply.github.com

wrote:

Already talked about ages ago but it seems we didn't make an issue for it.

ie check out dwarf land:

DomL has linked directly to the photo which is cool but would be better to just embed the photo, and link to the photo summary page instead of the raw photo.

http://www.thecrag.com/area/11812045

the other example from ages ago:

http://www.thecrag.com/area/11872453


Reply to this email directly or view it on GitHub: https://github.com/theCrag/website/issues/511

Campbell

brendanheywood commented 12 years ago

Originally I think simon and I kicked around some ideas like:

[27000084]

[27000084:right]

or something like that. We may want some flexibility with alignment and adding titles and other stuff but perhaps the simplest case is that we don't allow any fleibility and just replace photo url's with embedded photos.

ie this:

https://dev.thecrag.com/photo/27000084?size=fixedwidthmedium

would be replaced with the embedded photo. The size would be stripped out and then the best size for the medium chosen automatically.

When we add the 'stream' stuff to the photo urls that will just get ignored too.

This is also pretty similar to the oembed stuff #288 for embedding video or whatever from 3rd party sites.

cgome commented 12 years ago

Yeah the simplest one is the best

On Sat, Apr 21, 2012 at 3:05 PM, Brendan Heywood < reply@reply.github.com

wrote:

Originally I think simon and I kicked around some ideas like:

[27000084]

[27000084:right]

or something like that. We may want some flexibility with alignment and adding titles and other stuff but perhaps the simplest case is that we don't allow any fleibility and just replace photo url's with embedded photos.

ie this:

https://dev.thecrag.com/photo/27000084?size=fixedwidthmedium

would be replaced with the embedded photo. The size would be stripped out and then the best size for the medium chosen automatically.

When we add the 'stream' stuff to the photo urls that will just get ignored too.

This is also pretty similar to the oembed stuff #288 for embedding video or whatever from 3rd party sites.


Reply to this email directly or view it on GitHub: https://github.com/theCrag/website/issues/511#issuecomment-5258169

Campbell

brendanheywood commented 12 years ago

and I just realised my last comment kinda looked like I was just repeating your because I missed the point I was trying to make, which is that we should probably only embed pictures that are hosted with us. We could reference and embed images from other sites but it kinda side steps copyright and licene stuff, and can also be a security hole and we can't control the size and compression etc

cgome commented 12 years ago

Good call

On Sat, Apr 21, 2012 at 3:51 PM, Brendan Heywood < reply@reply.github.com

wrote:

and I just realised my last comment kinda looked like I was just repeating your because I missed the point I was trying to make, which is that we should probably only embed pictures that are hosted with us. We could reference and embed images from other sites but it kinda side steps copyright and licene stuff, and can also be a security hole and we can't control the size and compression etc


Reply to this email directly or view it on GitHub: https://github.com/theCrag/website/issues/511#issuecomment-5258334

Campbell

scd commented 12 years ago

see #385

scd commented 12 years ago

We need to make some business decisions.

Do we want to embed external third party images? Issues with doing so include:

I think we do NOT allow embedding of third party images as part of our standard markup. The only place where this may be relevant is in the forums, where a lot of these issues disappear, but we still need the forums to work in mobile apps.

If we are going to embed internal images then what is the best way to do this? The particular issue I see is that it MUST work for PDFs, in which case we need to standardise sizes. In other words we cannot let the user choose the specific size?

Can we get away with two standard embed sizes (thumbnails for portrait and landscape)?

What sort of positioning controls do we want the embed string to have in both the web and PDF versions?

How does the user work out the embed string? If they need the id of the photo then we may as well produce the embed string on the photo page so they can copy and paste it

What about text markup. A feature of thecrag markdown is that we also have a text version of the marked up output, so we definitely need alt text.

Daring markup syntax is:

![Alt text](/path/to/img.jpg "Optional title")

I recon we should disable Daring markup by preprocessing and do something similar like:

![id:size position](alt text)

So the following examples may work:

![1234]   # would pick default landscape view of image
![1234:portrait] 
![1234:portrait right]
![1234](Some image text)

What about embedding topos?

scd commented 12 years ago

I'm going to sit on this for longer until we get a clearer business logic of what we want to achieve with multi-channels, PDF Web, etc.

scd commented 12 years ago

I just realised we cannot clash at all with any of Daring markdown, so my suggestion is not quite right. What about

[[id:size position]]("alt text")

Single bracket version like [1234] definitely clashes with standard markdown.

brendanheywood commented 11 years ago

Whatever we do should also work in the context of forum posts. There should be a nice GUI for inserting a random pic including just dragging it into the post, like github posting.

brendanheywood commented 10 years ago

Example in the wild in this long discussion, someone uploaded a photo to use and copied the link:

http://www.thecrag.com/discussion/412205532/bolts-on-trad-route#m414111450

Mockup of how it should show in forum:

image

scd commented 10 years ago

For experimental purposes I am trialing a format

[[1234]]

full

[[1234:dimension modifiers]]"some title"

This is in repo ready for testing. Note that [[1234:dimension modifiers]]("some title") is translated to a link using standard markdown so we cannot use it.

It defaults to fixedwidthsmall dimension which seems about right for descriptions and forums.

No UI uploader yet

brendanheywood commented 10 years ago

What about the much more natural url type link?

If the stick with the markdown philosophy of it still working as raw text, this is quite an unnatural departure.

scd commented 10 years ago

We already convert natural urls to links. This is not part of the Daring Fireball syntax, but is extra processing we do. This means that it is possible. Questions:

Assuming that we are only doing it for internal photos (and topos) then it should be very doable.

Are you happy to commit to this?

brendanheywood commented 10 years ago

Yep that's all exactly what I had in mind. It will not just dump an img tag through it should dump a chunk of html, which includes the image with title and caption etc, ie call the minor/linkPhoto template. A topo url should call minor/phototopo which dumps all the topo data declaratively and then common.js reads this and turns it into a real topo. Thinking of other user cases pretty well anything could work, account url => minor/cardClimber, area url => minor/linkArea, maps, publications etc

This is pretty well the precursor to a oembed implementation (see #1095), both as a consumer of content and a producer. In this case we are both simultaneously. I am certainly not closed to accepting urls to external things, like photos, videos, etc and turning them into an embedded images and youtube videos etc, it's just that there is more work to do to do it safely, usually sticking it in an iframe and serving from another domain. There are oembed frameworks out there that can take a lot off this pain away, eg:

http://search.cpan.org/~miyagawa/Web-oEmbed/lib/Web/oEmbed.pm

External images in particular are a special case, the safest way to do it is to proxy them, which is why I'd let that use case stew for a while, see how github does it: https://github.com/blog/743-sidejack-prevention-phase-3-ssl-proxied-assets

Later when we want to become an oembed producer, eg embed a climber card onto someones blog, or a topo on someone elses page, this is pretty trivial, just pass the url into the expandInternalLink function inside Markdown.pm, and then wrap the output in a couple lines of boilerplate html and stick it in an iframe and were done. I don't think this is actually this much work, so I think I'll knock up a spec in a new issue and link to this. It would be a good proof that it is all working smoothly.

scd commented 10 years ago

I have got something working for this in repo, although it does not call linkPhoto template at this stage - circular reference problems which need a bit of sorting out.

The url needs to have thecrag.com or www.thecrag.com in it - keep this in mind when you are testing in dev.

Both the following urls will work,

thecrag.com/photo/1234 thecrag.com/image/photo/00001/modified/1234.jpg

The later will embed the specified dimension size.

brendanheywood commented 10 years ago

I just tried this inside the gym article, a node description and a forum and it didn't work anywhere. Pulled and deployed. Can you see why these don't work?

http://dev.thecrag.com/article/GymTicking

http://dev.thecrag.com/climbing/australia/coles-bay/area/11851621

http://dev.thecrag.com/discussion/330620305/coastal-cliffs--image-test

scd commented 10 years ago

Look at coles bay now. You need to have a real photo id.

brendanheywood commented 10 years ago

well doh! I thought your example was real, ok trying again...

Ok working but run into a few things, I've downloaded and re-uploaded the 'ask your gym...' image, and it's working except the high quality png has been converted to a fairly crappy jpg with a black background. I think for more forum pics this won't be an issue, and for most article pics too like the hand holding the iphone. But I think a really simple work around for this situation is a hack for urls which are to static images, and just ref them blindly without doing any lookup. eg:

http://dev.thecrag.com/static/cids/images/gym-1.1.0/ask-your-gym-about-ticking.png

The next challenge is the sizing and float layout etc. For forums, descs, tick comments, I'm kinda happy to keep this really simple and just not really give people the option. The layout should be dependant on the context it is shown in and not the context of who and where it was authored. So even if the author embedded a url of a 'large' picture vs a 'modified' one they should come out exactly the same. The images are already responsive so right now the two pics of 'rose ramble' you added appear mostly the same on mobile, it's just one wastes a lot more bandwidth. On the layout side this should also be dependant on how you consume it, ie on a real big wide screen where the pic is less than say 40% of the text width then it should float and wrap, but under that it should unfloat and be centered. We don't yet have a proper responsive image solution which loads different pics based on size, but when we do that we can retrofit it. In the mean time I think we should just pick a middle ground like 'modified' and always show that.

So this only leaves special layout cases for help articles. Given that there are already lots of special cases for the help I reckon just leave it as is, or rework them into a simpler format where we can.

scd commented 10 years ago

I am happy with your suggestions for forums and area descriptions.

The thing we need to spend a little bit more time on is a better workflow for the articles. At the moment the workflow is for me to update on dev and release as part of the next upgrade cycle.

We need a workable solution for uploading images for articles. Maybe we need to somehow make the high quality image available for articles, rather then the crappy quality one.

brendanheywood commented 10 years ago

This only really applies to diagram style images vs photos. What is the real goal for the help articles after this bit of work anyway? Now that we can accept markdown in the article pages it's much easier for us to hack in google docs and then cut and paste as-is with no html processing directly into the article and were done. It's still a manual process but it's a good enough 90/10 solution. And it avoids the whole 'what do we do with grade tables' and related issues. and we can slowly migrate each article from html to markdown as we need to edit them

scd commented 10 years ago

so here is my loose progression of what I want to achieve

scd commented 10 years ago

I think this is done. Can we close this?

brendanheywood commented 10 years ago

closing - exaple above works great