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.

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

Fix: Prevent overwrite of cached page on presentation preview init #916

Merged
merged 4 commits into from
Feb 11, 2019
Merged

Fix: Prevent overwrite of cached page on presentation preview init #916

merged 4 commits into from
Feb 11, 2019

Conversation

jeremypress
Copy link
Contributor

@jeremypress jeremypress commented Feb 6, 2019

todo:

  • test on mobile
  • add e2e test

@boxcla
Copy link

boxcla commented Feb 6, 2019

Verified that @jeremypress has signed the CLA. Thanks for the pull request!

ConradJChan
ConradJChan previously approved these changes Feb 6, 2019
ConradJChan
ConradJChan previously approved these changes Feb 7, 2019
Copy link
Contributor

@ConradJChan ConradJChan left a comment

Choose a reason for hiding this comment

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

minor comments, otherwise LGTM

@@ -0,0 +1,35 @@
// <reference types="Cypress" />
describe('Preview Document Controls', () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Update the suite name?

beforeEach(() => {
cy.visit('/');
cy.showPreview(token, fileId);
cy.getByTestId('current-page').as('currentPage');
Copy link
Contributor

Choose a reason for hiding this comment

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

This doesn't seem to be used?

ConradJChan
ConradJChan previously approved these changes Feb 8, 2019
jstoffan
jstoffan previously approved these changes Feb 8, 2019
const fileId = Cypress.env('FILE_ID_PRESENTATION');

/* eslint-disable */
const showControls = () => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this is duplicated in other test files. Should it be pulled out as a top-level helper?

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

test/integration/document/PresentationViewer.e2e.test.js Outdated Show resolved Hide resolved
test/integration/document/PresentationViewer.e2e.test.js Outdated Show resolved Hide resolved
JustinHoldstock
JustinHoldstock previously approved these changes Feb 8, 2019
@jeremypress jeremypress merged commit 9256dcc into box:master Feb 11, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants