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.

Sign up for GitHub

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

Jump to bottom

show pdf preview in the files sidebar #15

Merged
merged 1 commit into from
Dec 2, 2016
Merged

show pdf preview in the files sidebar #15

merged 1 commit into from
Dec 2, 2016

Conversation

icewind1991
Copy link
Member

@icewind1991 icewind1991 commented Dec 1, 2016 •
edited
Loading

@icewind1991 icewind1991 mentioned this pull request Dec 1, 2016
Merged
nickvergessen
nickvergessen reviewed Dec 1, 2016
View reviewed changes
js/previewplugin.js
(function () {

var SidebarPreview = function () {
this.preview = new OCA.Files_Markdown.Preview();
Copy link
Member

Choose a reason for hiding this comment

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

Inter app dependency or mistake?

Copy link
Member Author

Choose a reason for hiding this comment

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

I would never just copy-past a block of code and forget to properly clean things up

nickvergessen
nickvergessen reviewed Dec 1, 2016
View reviewed changes
js/previewplugin.js
},

handlePreview: function (model, $thumbnailDiv, $thumbnailContainer, fallback) {
console.log('b');
Copy link
Member

Choose a reason for hiding this comment

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

🙊

@icewind1991
Copy link
Member Author

@nickvergessen fixed

@LukasReschke
Copy link
Member

I get an error:

Uncaught TypeError: Cannot read property 'init' of undefined(…)SidebarPreview @ previewplugin.js?v=0ff9905…:153(anonymous function) @ previewplugin.js?v=0ff9905…:184(anonymous function) @ previewplugin.js?v=0ff9905…:185

@icewind1991 icewind1991 added this to the Nextcloud 11.0 milestone Dec 1, 2016
@jancborchardt
Copy link
Member

Similar here:

TypeError: this.preview is undefined in previewplugin.js:153:3

Btw when this works, we have another widely used file type ready for a possible grid view, right?

@LukasReschke
Copy link
Member

Btw when this works, we have another widely used file type ready for a possible grid view, right?

No. I doubt that this will scale at all. We need a server-side preview generation for that.

@icewind1991
show pdf preview in the files sidebar
2a8cde9 
Signed-off-by: Robin Appelman <robin@icewind.nl>
@MorrisJobke
Copy link
Member

Ummm what? We simply load the PDF in the side bar? :)

This will not work for a grid view in my opinion. At least once you have something like 10 PDFs in a folder it will get tricky.

@icewind1991
Copy link
Member Author

icewind1991 commented Dec 1, 2016 •
edited
Loading

Fixed the error

I agree that this should not be used for grid

@MorrisJobke
Copy link
Member

I agree that this should be used for grid

I guess there is a not missing ;)

@MorrisJobke
Copy link
Member

Tested and works 👍 but we should not use this in a grid view.

@LukasReschke LukasReschke merged commit 93f3ad3 into master Dec 2, 2016
@LukasReschke LukasReschke deleted the sidebare-preview branch December 2, 2016 10:34
@icewind1991
Copy link
Member Author

I guess there is a not missing ;)

Correct

@jancborchardt
Copy link
Member

Okidoke, thanks for clarification :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nickvergessen nickvergessen nickvergessen left review comments

Assignees
No one assigned
Labels
3. to review
Projects
None yet
Milestone
Nextcloud 11.0
Development

Successfully merging this pull request may close these issues.
5 participants
@icewind1991 @LukasReschke @jancborchardt @MorrisJobke @nickvergessen