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

feat: display hero image in news carousel #901

Merged
merged 4 commits into from
Jan 17, 2023

Conversation

gidjin
Copy link
Contributor

@gidjin gidjin commented Jan 9, 2023

SC-1394

Proposed changes

Articles can have hero images. Those are displayed when viewing an article on the single article page. This PR updates it so that the internal news carousel uses the hero image if it is available. If it is missing the current placeholder that uses the USSF logo will still be displayed.

Reviewer notes

The hero image has a max height of 534px to match the height of the design in Figma.

Setup

Start up storybook yarn storybook and view the NewsCarouselItem with Hero component

Start up the servers

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

Create two internal articles in the cms one with a hero image and one without. Be sure to select InternalNews as the category. View the carousel on the news page one should have the default hero image and one should have the custom one. See screenshots below.


Code review steps

As the original developer, I have

  • Met the acceptance criteria, or will meet them in subsequent PRs or stories
    • ...
  • Created new stories in Storybook if applicable
    • Checked that all Storybook accessibility checks are passing
  • Created/modified automated unit tests in Jest
    • Including jest-axe checks when UI changes
  • Created/modified automated E2E tests in Cypress
    • Including cypress-axe checks when UI changes
    • Checked that the E2E test build is not failing
  • Performed a11y testing:
    • Checked responsiveness in mobile, tablet, and desktop
    • Checked keyboard navigability
    • Tested with VoiceOver in Safari
    • Checked VO's rotor menu for landmarks, page heading structure and links
    • Used a browser a11y tool to check for issues (WAVE, axe, ANDI or Accessibility addon tab for Storybook)
  • Requested a design review for user-facing changes
  • For any new migrations/schema changes:
    • Followed guidelines for zero-downtime deploys

As code reviewer(s), I have

  • Pulled this branch locally and tested it
  • Reviewed this code and left comments
  • Checked that all code is adequately covered by tests
    • Checked that the E2E test build is not failing
  • Made it clear which comments need to be addressed before this work is merged
  • Considered marking this as accepted even if there are small changes needed
  • Performed a11y testing:
    • Checked responsiveness in mobile, tablet, and desktop
    • Checked keyboard navigability
    • Tested with VoiceOver in Safari
    • Checked VO's rotor menu for landmarks, page heading structure and links
    • Used a browser a11y tool to check for issues (WAVE, axe, ANDI or Accessibility addon tab for Storybook)

As a designer reviewer, I have

  • Checked in the design translated visually
  • Checked behavior
  • Checked different states (empty, one, some, error)
  • Checked for landmarks, page heading structure, and links
  • Tried to break the intended flow
  • Performed a11y testing:
    • Checked responsiveness in mobile, tablet, and desktop
    • Checked keyboard navigability
    • Tested with VoiceOver in Safari
    • Checked VO's rotor menu for landmarks, page heading structure and links
    • Used a browser a11y tool to check for issues (WAVE, axe, ANDI or Accessibility addon tab for Storybook)

As a test user, I have

  • Run through the Test Script:
    • On commercial internet in IE11
    • On commercial internet in Firefox
    • On commercial internet in Chrome
    • On commercial internet in Safari
    • On NIPR in IE11
    • On NIPR in Firefox
    • On NIPR in Chrome
    • On NIPR in Safari
    • On a mobile device in Firefox
    • On a mobile device in Chrome
    • On a mobile device in Safari
  • Added any anomalous behavior to this PR

Screenshots

app with hero image

SCR-20230109-kd9

app without hero image

SCR-20230109-kdl

storybook

SCR-20230109-kcp

SCR-20230109-kav

@shortcut-integration
Copy link

This pull request has been linked to Shortcut Story #1394: Display uploaded images for carousel thumbnail.

@gidjin gidjin requested review from jcbcapps and a team January 9, 2023 22:42
@gidjin gidjin force-pushed the sc-1394/display_hero_image_in_carousel branch from 3734bef to ba70a39 Compare January 9, 2023 22:44
@shkeating
Copy link
Contributor

looks good to me 🚀

@shkeating shkeating requested a review from a team as a code owner January 10, 2023 21:56
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.

Looks great!

Copy link
Contributor

@abbyoung abbyoung left a comment

Choose a reason for hiding this comment

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

LGTM! Works as expected 🎉

@gidjin gidjin merged commit 34fefac into main Jan 17, 2023
@gidjin gidjin deleted the sc-1394/display_hero_image_in_carousel branch January 17, 2023 17:40
@gidjin gidjin mentioned this pull request Jan 25, 2023
gidjin added a commit that referenced this pull request Jan 26, 2023
## [4.13.0](4.12.1...4.13.0) (2023-01-25)


### Features

* display hero image in news carousel ([#901](#901)) ([34fefac](34fefac))


### Bug Fixes

* **deps:** update dependency micro to v10 ([#879](#879)) ([c538652](c538652))
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