remove viewer page div id #7971
Conversation
There was a problem hiding this comment.
I'm wondering if, for consistency, we should make the same change in pdf_thumbnail_view.js as well?
I.e. do we want to remove line pdf_thumbnail_view.js#L127, and add
div.setAttribute('data-page-number', this.id);before line pdf_thumbnail_view.js#L129?
|
@Snuffleupagus thanks! I've added the removal of the .id and addition of the data-page-number attribute for thumbnail divs. I checked in the mozilla-central tests and did not find any reference to the thumbnails' ids (the two tests of the thumbnails use nth-child selectors: https://dxr.mozilla.org/mozilla-central/source/browser/extensions/pdfjs/test/browser_pdfjs_navigation.js#49,57), and the tests pass locally for me with the id removed. |
|
Looks good. Could you squash the two commits into one commit (refer to https://github.com/mozilla/pdf.js/wiki/Squashing-Commits for how to do this easily)? |
davehouse
force-pushed
the
remove_viewer_page_div_id
branch
from
b4d50f5
to
a4a2930
Compare
| @@ -124,8 +124,8 @@ var PDFThumbnailView = (function PDFThumbnailViewClosure() { | |||
| this.anchor = anchor; | |||
|
|
|||
| var div = document.createElement('div'); | |||
| div.id = 'thumbnailContainer' + id; | |||
There was a problem hiding this comment.
There is code at
pdf.js/web/pdf_thumbnail_viewer.js
Line 90 in e9c63a2
data-page-number attribute as well.
There was a problem hiding this comment.
Thank you. I added that change and squashed it in 👍
davehouse
force-pushed
the
remove_viewer_page_div_id
branch
from
51b18e8
to
eeeccb0
Compare
| @@ -87,7 +87,7 @@ var PDFThumbnailViewer = (function PDFThumbnailViewerClosure() { | |||
| if (selected) { | |||
| selected.classList.remove('selected'); | |||
| } | |||
| var thumbnail = document.getElementById('thumbnailContainer' + page); | |||
| var thumbnail = document.querySelector('div.thumbnail[data-page-number="' + page + '"]'); | |||
There was a problem hiding this comment.
This line exceeds the maximum length of 80 characters, which causes a linting failure. To resolve this, please change it to:
var thumbnail = document.querySelector(
'div.thumbnail[data-page-number="' + page + '"]');
davehouse
force-pushed
the
remove_viewer_page_div_id
branch
from
eeeccb0
to
5236069
Compare
The id, and parsing it for the page number, is replaced by the data-page-number attribute (added here for thumbnails).
|
Thank you! |
|
/botio-linux preview |
From: Bot.io (Linux)ReceivedCommand cmd_preview from @timvandermeij received. Current queue size: 0 Live output at: http://107.21.233.14:8877/730171d76776889/output.txt |
From: Bot.io (Linux)SuccessFull output at http://107.21.233.14:8877/730171d76776889/output.txt Total script time: 2.25 mins Published |
|
Nice work! |
remove viewer page div id
This removes the page div id from the viewer. The id, and parsing it for the page number, is replaced by the data-page-number attribute.
The mozilla-cental test that depended on the div.id has been updated to use data-page-number (checked-in for Target Milestone: Firefox 53, Bug 1331795).
This resolves #5897