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

Update: Wider thumbnails #963

Merged
merged 7 commits into from
Mar 26, 2019
Merged

Update: Wider thumbnails #963

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
91b87c1
Update: Increase thumbnails width
Mar 25, 2019
74c0d11
Chore: update e2e test
Mar 25, 2019
17d8778
Chore: fix unit tests and address PR comments
Mar 25, 2019
014997f
Chore: update e2e assertions
Mar 26, 2019
e401942
Merge branch 'master' into wider-thumbnails
Mar 26, 2019
2adafb9
Chore: adding thumbnail-nav width back
Mar 26, 2019
35bf279
Merge branch 'wider-thumbnails' of github.com:ConradJChan/box-content…
Mar 26, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions src/lib/ThumbnailsSidebar.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,9 +8,9 @@ const CLASS_BOX_PREVIEW_THUMBNAIL_IMAGE = 'bp-thumbnail-image';
const CLASS_BOX_PREVIEW_THUMBNAIL_IMAGE_LOADED = 'bp-thumbnail-image-loaded';
const CLASS_BOX_PREVIEW_THUMBNAIL_IS_SELECTED = 'bp-thumbnail-is-selected';
const CLASS_BOX_PREVIEW_THUMBNAIL_PAGE_NUMBER = 'bp-thumbnail-page-number';
const DEFAULT_THUMBNAILS_SIDEBAR_WIDTH = 127; // 200px sidebar width - 25px margin right, - 40px for page number - 8px for border
export const DEFAULT_THUMBNAILS_SIDEBAR_WIDTH = 154; // 225px sidebar width - 25px margin right, - 40px for page number - 6px for border
const THUMBNAIL_MARGIN = 15;
const BORDER_WIDTH = 8;
const BORDER_WIDTH = 6;

class ThumbnailsSidebar {
/** @property {HTMLElement} - The anchor element for this ThumbnailsSidebar */
Expand Down
11 changes: 6 additions & 5 deletions src/lib/__tests__/ThumbnailsSidebar-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,9 @@
/* eslint-disable no-unused-expressions */
import ThumbnailsSidebar from '../ThumbnailsSidebar';
import ThumbnailsSidebar, { DEFAULT_THUMBNAILS_SIDEBAR_WIDTH } from '../ThumbnailsSidebar';
import VirtualScroller from '../VirtualScroller';

const sandbox = sinon.sandbox.create();
const TEST_SCALE = DEFAULT_THUMBNAILS_SIDEBAR_WIDTH / 10;

describe('ThumbnailsSidebar', () => {
let thumbnailsSidebar;
Expand Down Expand Up @@ -102,7 +103,7 @@ describe('ThumbnailsSidebar', () => {

return pagePromise.then(() => {
expect(stubs.getViewport).to.be.called;
expect(thumbnailsSidebar.scale).to.be.equal(12.7); // DEFAULT_THUMBNAILS_SIDEBAR_WIDTH / width
expect(thumbnailsSidebar.scale).to.be.equal(TEST_SCALE); // DEFAULT_THUMBNAILS_SIDEBAR_WIDTH / width
expect(thumbnailsSidebar.pageRatio).to.be.equal(1);
expect(stubs.vsInit).to.be.called;
});
Expand Down Expand Up @@ -209,6 +210,7 @@ describe('ThumbnailsSidebar', () => {
.returns(createImagePromise);
stubs.appendChild = sandbox.stub();
stubs.addClass = sandbox.stub();
stubs.renderNextThumbnailImage = sandbox.stub(thumbnailsSidebar, 'renderNextThumbnailImage');

const thumbnailEl = {
lastChild: { appendChild: stubs.appendChild },
Expand Down Expand Up @@ -281,7 +283,7 @@ describe('ThumbnailsSidebar', () => {
stubs.getViewport.withArgs(1).returns({ width: 10, height: 10 });
stubs.render.returns(Promise.resolve());

const expScale = 12.7; // Should be DEFAULT_THUMBNAILS_SIDEBAR_WIDTH / 10
const expScale = TEST_SCALE; // Should be DEFAULT_THUMBNAILS_SIDEBAR_WIDTH / 10

return thumbnailsSidebar.getThumbnailDataURL(1).then(() => {
expect(stubs.getPage).to.be.called;
Expand All @@ -298,8 +300,7 @@ describe('ThumbnailsSidebar', () => {
stubs.getViewport.withArgs(1).returns({ width: 10, height: 20 });
stubs.render.returns(Promise.resolve());

const expScale = 6.3; // Should be 6.3 instead of 12.7 because the viewport ratio above is 0.5 instead of 1
// and because canvas width is integer, so 63.5 becomes 63
const expScale = TEST_SCALE / 2; // Should be DEFAULT_THUMBNAILS_SIDEBAR_WIDTH / 10 / 2

return thumbnailsSidebar.getThumbnailDataURL(0).then(() => {
expect(stubs.getPage).to.be.called;
Expand Down
7 changes: 3 additions & 4 deletions src/lib/viewers/doc/_docBase.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,8 @@
@import '../../boxuiVariables';

$thumbnail-border-radius: 4px;
// Accounts for the 1px border on the right so the remaining width is actually 200
$thumbnail-sidebar-width: 201px;
$thumbnail-border-radius: 3px;
// Accounts for the 1px border on the right so the remaining width is actually 225
$thumbnail-sidebar-width: 226px;

.bp {
.bp-thumbnails-container {
Expand Down Expand Up @@ -55,7 +55,6 @@ $thumbnail-sidebar-width: 201px;
flex: 1 0 auto;
overflow: hidden;
padding: 0;
width: 134px;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Was this not needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah I don't think so because the flexbox width seems to take precedence

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I take it back, ie11 needs this, but I'll set it to the flex-basis instead

}

.bp-thumbnail-image {
Expand Down
37 changes: 21 additions & 16 deletions test/integration/document/Thumbnails.e2e.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ describe('Preview Document Thumbnails', () => {
const token = Cypress.env('ACCESS_TOKEN');
const fileId = Cypress.env('FILE_ID_DOC_LARGE');
const THUMBNAIL_SELECTED_CLASS = 'bp-thumbnail-is-selected';
const THUMBNAILS_OPEN = 'bp-thumbnails-open';

/**
* Gets the thumbnail with the specified page number
Expand Down Expand Up @@ -52,19 +53,15 @@ describe('Preview Document Thumbnails', () => {

/**
* Asserts that the thumbnails sidebar object is visible
* @param {Element} $thumbnailsSidebar - The thumbnails sidebar subject
* @return {Assertion} Chai Assertion
* @return {Element} Preview element
*/
const beVisible = ($thumbnailsSidebar) =>
expect($thumbnailsSidebar).to.have.css('transform', 'matrix(1, 0, 0, 1, 0, 0)'); // translateX(0)
const verifyThumbnailsVisible = () => cy.getByTestId('bp').should('have.class', THUMBNAILS_OPEN);

/**
* Asserts that the thumbnails sidebar object is not visible
* @param {Element} $thumbnailsSidebar - The thumbnails sidebar subject
* @return {Assertion} Chai Assertion
* @return {Element} Preview element
*/
const notBeVisible = ($thumbnailsSidebar) =>
expect($thumbnailsSidebar).to.have.css('transform', 'matrix(1, 0, 0, 1, -201, 0)'); // translateX(-201px)
const verifyThumbnailsNotVisible = () => cy.getByTestId('bp').should('not.have.class', THUMBNAILS_OPEN);

beforeEach(() => {
cy.visit('/');
Expand All @@ -87,9 +84,11 @@ describe('Preview Document Thumbnails', () => {
it('Should render thumbnails when toggled', () => {
showDocumentPreview({ enableThumbnailsSidebar: true });

toggleThumbnails().should(beVisible);
toggleThumbnails();
verifyThumbnailsVisible();

toggleThumbnails().should(notBeVisible);
toggleThumbnails();
verifyThumbnailsNotVisible();
});

it('Should be able to change page by clicking on the thumbnail', () => {
Expand All @@ -102,7 +101,8 @@ describe('Preview Document Thumbnails', () => {
.invoke('text')
.should('equal', '1');

toggleThumbnails().should(beVisible);
toggleThumbnails();
verifyThumbnailsVisible();

// Verify which thumbnail is selected
getThumbnailWithRenderedImage(1).should('have.class', THUMBNAIL_SELECTED_CLASS);
Expand All @@ -124,7 +124,8 @@ describe('Preview Document Thumbnails', () => {
.invoke('text')
.should('equal', '1');

toggleThumbnails().should(beVisible);
toggleThumbnails();
verifyThumbnailsVisible();

getThumbnailWithRenderedImage(1).should('have.class', THUMBNAIL_SELECTED_CLASS);

Expand All @@ -146,7 +147,8 @@ describe('Preview Document Thumbnails', () => {
.invoke('text')
.should('equal', '1');

toggleThumbnails().should(beVisible);
toggleThumbnails();
verifyThumbnailsVisible();

getThumbnailWithRenderedImage(1).should('have.class', THUMBNAIL_SELECTED_CLASS);

Expand Down Expand Up @@ -175,7 +177,8 @@ describe('Preview Document Thumbnails', () => {
.invoke('text')
.should('equal', '1');

toggleThumbnails().should(beVisible);
toggleThumbnails();
verifyThumbnailsVisible();

getThumbnailWithRenderedImage(1).should('have.class', THUMBNAIL_SELECTED_CLASS);

Expand All @@ -190,7 +193,8 @@ describe('Preview Document Thumbnails', () => {
cy.getPreviewPage(200).should('be.visible');
getThumbnailWithRenderedImage(200).should('have.class', THUMBNAIL_SELECTED_CLASS);

toggleThumbnails().should(notBeVisible);
toggleThumbnails();
verifyThumbnailsNotVisible();

cy.getByTitle('Click to enter page number').click();
cy
Expand All @@ -201,7 +205,8 @@ describe('Preview Document Thumbnails', () => {

cy.getPreviewPage(1).should('be.visible');

toggleThumbnails().should(beVisible);
toggleThumbnails();
verifyThumbnailsVisible();

getThumbnailWithRenderedImage(1).should('have.class', THUMBNAIL_SELECTED_CLASS);
});
Expand Down