wants to merge 5 commits
Please review and suggest changes.
Update test_forum:testposts test with updated html
Updated code as per suggestions in review
Updated Tests to match changes
Readd old attachment css
Initial design changes to list attachments. Added lightbox_me to view images
Looks a lot better :D
It would be great to have this applied to other attachment areas, like wiki pages and tickets. That's fine to be done later separately though.
Will do the other areas in a different mr.
What do you think of opening PDFs in an iframe? Although I don't know if this will be compatible with all browsers.
I think PDFs can have security risks since they can have scripts embedded in them (but I'm not an expert). So we'd have to host them on a domain separate from the main domain. Which obviously would require extra setup and configuration, like having Allura running and responding to an additional domain somehow. Or possibly store attachments on disk ([#2560]) and then run a simple webserver for them, instead of Allura serving them.
I'm also not sure about the security aspect. Could you point me to some keywords so I can search more about this.
Also wanted a clarification regarding [#2560] and [#6390], from what I understand you are suggesting that we should upload to a server and store the link to the file in the db instead of the file, right?
Regarding the compatibility, I had found a MIT Licensed library that handles that https://pdfobject.com/ and also mozilla's https://mozilla.github.io/pdf.js/web/viewer.html (https://github.com/mozilla/pdf.js/wiki/Frequently-Asked-Questions). Would these be satisfying our requirements? (sample code: https://github.com/mozilla/pdf.js/tree/master/examples/helloworld)
For 2560/6390 yes, but I think that's a lot of extra work and probably not worth pursuing at this point, in my opinion.
pdf.js is a good idea. It might not allow JS to run from within a PDF. But hard to say, since we don't know how PDF+JS works.
forgediscussion.tests.functional.test_forum:TestForumAsync.test_posts is failing. Otherwise looks good.
I didn't know about the download attribute, that is nice. Makes [#6132] mostly taken care of (except for allowing text attachments to be displayed without download)
Updated the test. Yes I guess [#6132] is taken care of. I will look into the PDF problem it a bit further this weekend.
Added another commit to fix JS from running too much and making it difficult to close the lightboxes: [cd3b40]
Log in to post a comment.