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

Convert the attachments/outline view to ES6 syntax #8302

Merged
merged 1 commit into from
Apr 23, 2017
Merged

Convert the attachments/outline view to ES6 syntax #8302

merged 1 commit into from
Apr 23, 2017

Conversation

timvandermeij
Copy link
Contributor

@timvandermeij timvandermeij commented Apr 17, 2017 •
edited
Loading

Snuffleupagus
Snuffleupagus reviewed Apr 17, 2017
View reviewed changes
Copy link
Collaborator

@Snuffleupagus Snuffleupagus left a comment

Choose a reason for hiding this comment

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

I've not had time to test the PR yet, but I'm adding a couple of small comments/questions based on a cursory look at the diff.

web/pdf_attachment_viewer.js Outdated
PDFAttachmentViewer.prototype = {
reset: function PDFAttachmentViewer_reset(keepRenderedCapability) {
this.attachments = null;
reset(keepRenderedCapability) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nit: reset(keepRenderedCapability = false) {.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done in the new commit.

web/pdf_attachment_viewer.js Outdated
*/
_bindPdfLink(button, content, filename) {
var blobUrl;
button.onclick = () => {
Copy link
Collaborator

@Snuffleupagus Snuffleupagus Apr 17, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Nit: Unless I'm missing something, I don't think that you need an arrow function here (no this in the function scope)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done in the new commit.

web/pdf_attachment_viewer.js Outdated
return;
}

var names = Object.keys(attachments).sort((a, b) => {
Copy link
Collaborator

@Snuffleupagus Snuffleupagus Apr 17, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Nit: Unless I'm missing something, I don't think that you need an arrow function here (no this in the function scope)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done in the new commit.

Snuffleupagus
Snuffleupagus reviewed Apr 17, 2017
View reviewed changes
web/pdf_attachment_viewer.js Outdated
/**
* @param {PDFAttachmentViewerRenderParameters} params
*/
render(params = Object.create(null)) {
Copy link
Collaborator

@Snuffleupagus Snuffleupagus Apr 17, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

It's a matter of personal preference, but to me this feels overly verbose, and I think that just render(params = {}) { would suffice.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done in the new commit.

Snuffleupagus
Snuffleupagus reviewed Apr 17, 2017
View reviewed changes
web/pdf_outline_viewer.js Outdated
/**
* @param {PDFOutlineViewerRenderParameters} params
*/
render(params = Object.create(null)) {
Copy link
Collaborator

@Snuffleupagus Snuffleupagus Apr 17, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

It's a matter of personal preference, but to me this feels overly verbose, and I think that just render(params = {}) { would suffice.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done in the new commit.

@timvandermeij
Copy link
Contributor Author

/botio-linux preview

Snuffleupagus
Snuffleupagus reviewed Apr 18, 2017
View reviewed changes
Copy link
Collaborator

@Snuffleupagus Snuffleupagus left a comment •
edited
Loading

Choose a reason for hiding this comment

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

This could now become const instead: https://github.com/mozilla/pdf.js/blob/master/web/pdf_outline_viewer.js#L20.

@timvandermeij Please note that the following is not a criticism of your nice work in this patch, transitioning more /web code to proper classes!
I'm simply interested in your thoughts on this matter, and whether you think we should try to avoid using e as an identifier?

One thing that I've been guilty of myself from time to time, is to use e as a variable/argument name.
However, I'm beginning to think that using one-character names like this isn't really conducive to readability of the code.
Particularly in the case of the e shorthand-name, unless you look at the code, there can be some confusion as to what it represents. Looking at our code-base, it seems that it can be shorthand for either error, exception, or event, depending on the context.
I see why one might not, depending on the situation, always want to spell out full names. However, something like err, ex, or evt (for the cases above) might at least help readability somewhat!?

@timvandermeij
Copy link
Contributor Author

timvandermeij commented Apr 18, 2017 •
edited
Loading

I don't like one-character variable names in general, but for some reason e has always been the one exception for me because I didn't really see how it could be interpreted differently and it's being used all the time in many JS projects. However, now that you point it out, I can see that it may be confusing here. Personally I'm also more than fine with err, ex or evt, as long as we're consistent, which together with its general usage made it my main choice. I'll update the patches soon to change it to evt as that may indeed give a bit more readability.

@timvandermeij
Copy link
Contributor Author

timvandermeij commented Apr 23, 2017 •
edited
Loading

I have updated the patch to use const and to rename e to evt. Moreover, the patch is rebased onto the current master.

@timvandermeij
Copy link
Contributor Author

/botio-linux preview

@pdfjsbot
Copy link

From: Bot.io (Linux)

Received

Command cmd_preview from @timvandermeij received. Current queue size: 0

Live output at: http://107.21.233.14:8877/873061191c1e8d0/output.txt

@pdfjsbot
Copy link

From: Bot.io (Linux)

Success

Full output at http://107.21.233.14:8877/873061191c1e8d0/output.txt

Total script time: 3.24 mins

Published

Snuffleupagus
Snuffleupagus approved these changes Apr 23, 2017
View reviewed changes
Copy link
Collaborator

@Snuffleupagus Snuffleupagus left a comment

Choose a reason for hiding this comment

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

Looks great, thank you for doing this!

@Snuffleupagus Snuffleupagus merged commit ece24df into mozilla:master Apr 23, 2017
@timvandermeij timvandermeij deleted the es6-attachments-outline branch April 23, 2017 15:01
movsb pushed a commit to movsb/pdf.js that referenced this pull request Jul 14, 2018
@Snuffleupagus
Merge pull request mozilla#8302 from timvandermeij/es6-attachments-ou…
49f14a8 
…tline

Convert the attachments/outline view to ES6 syntax
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Snuffleupagus Snuffleupagus Snuffleupagus approved these changes

Assignees
No one assigned
Labels
viewer
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@timvandermeij @pdfjsbot @Snuffleupagus