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: Open document in browser #1116

Merged
merged 2 commits into from
Oct 11, 2023
Merged

fix: Open document in browser #1116

merged 2 commits into from
Oct 11, 2023

Conversation

abbyoung
Copy link
Contributor

@abbyoung abbyoung commented Oct 6, 2023

SC-2436

Proposed changes

  • Changes endsWith to includes when checking an asset url for the pdf extension
  • Adds a unit test to bump coverage

PDFs will now open in-browser. Other document formats will continue to download to a user's machine.

Reviewer notes

In addition to the small code change, we need to update the CORS settings on the s3 bucket. This change has been made on dev, so the feature should now work as expected on dev.ussforbit.us. @jbecker01 Let me know when you're ready to test and I can double check that the branch is available on dev!

When we're ready to deploy, we'll also need @minhlai's assistance to make the changes on staging and prod.

Setup

Start the system

yarn services:up
yarn dev
cd ../ussf-portal-cms
yarn dev

Login to the portal http://localhost:3000

Start storybook

yarn storybook

Login to storybook http://localhost:6006, though the command above should open it for you


Code review steps

As the original developer, I have

  • Met the acceptance criteria
  • Created new stories in Storybook if applicable
  • Created/modified automated unit tests in Jest
  • Created/modified automated E2E tests
  • Followed guidelines for zero-downtime deploys, if applicable
  • Use ANDI to check for basic a11y issues

As a reviewer, I have

Check out our How to review a pull request document.


Screenshots

@shortcut-integration
Copy link

@abbyoung abbyoung force-pushed the sc-2436-open-doc-in-browser branch from 9c64eec to 1540ee6 Compare October 6, 2023 22:25
@abbyoung abbyoung changed the title bug: [WIP] Open document in browser fix: [WIP] Open document in browser Oct 10, 2023
@abbyoung abbyoung marked this pull request as ready for review October 10, 2023 20:38
@abbyoung abbyoung requested a review from a team as a code owner October 10, 2023 20:38
@abbyoung abbyoung requested a review from jbecker01 October 10, 2023 20:39
@abbyoung abbyoung self-assigned this Oct 10, 2023
@abbyoung abbyoung changed the title fix: [WIP] Open document in browser fix: Open document in browser Oct 10, 2023
Copy link
Contributor

@jbecker01 jbecker01 left a comment

Choose a reason for hiding this comment

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

Looks good!

Copy link
Contributor

@jcbcapps jcbcapps left a comment

Choose a reason for hiding this comment

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

LGTM!

@minhlai minhlai merged commit b6e6696 into main Oct 11, 2023
@minhlai minhlai deleted the sc-2436-open-doc-in-browser branch October 11, 2023 21:21
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.

4 participants