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

feat: Display Spaceforce.mil news articles below the news carousel #746

Merged
merged 37 commits into from
Aug 23, 2022
Merged

feat: Display Spaceforce.mil news articles below the news carousel #746

merged 37 commits into from
Aug 23, 2022

Conversation

jcbcapps
Copy link
Contributor

@jcbcapps jcbcapps commented Aug 16, 2022 •
edited
Loading

SC-771

Proposed changes

  • Display latest 4 stories from the spaceforce.mil RSS news feed
  • Provide a link to the Spaceforce.mil news page below the articles

Reviewer notes

Note: this PR is built off of SC-589, so please review that PR first

Another note: I updated an e2e test in this repo, but also updated it in the ussf-portal repo and created a PR there. Please review that PR in tandem with this one.

Third and final note (hopefully): This branch also includes the (very few) updates needed to finish SC-772 so please give that story a glance.

Navigate to /news-announcements

  • Verify that only the latest four news articles from the RSS feed are being displayed
  • Verify that there is a button beneath the articles with the text View older Spaceforce.mil articles and that when clicked, takes you to https://www.spaceforce.mil/News in a new tab

Setup

Make sure to have both the CMS and the portal client running on your local machine. You can do this by

  • Running yarn services:up && yarn dev in the CMS repo and
  • Running yarn dev in the portal client with this branch checked out

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
  • 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
  • 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

jcbcapps and others added 26 commits August 11, 2022 15:48
@jcbcapps @abbyoung
Change current news path to news-announcments
dfd2ad8
@jcbcapps @abbyoung
Update test
e3cecb4
@jcbcapps @abbyoung
Add NewsCarousel and NewsCarouselItem components
bd02b3d
@jcbcapps @abbyoung
Update storybook component
03cdf50
@jcbcapps @abbyoung
Add CTA
728db43
@jcbcapps @abbyoung
Add query for carousel articles
66672cc
@jcbcapps @abbyoung
Add NewsCarousel component to page
ba3009e
@jcbcapps @abbyoung
Add custom ellipses and custom arrows
c657ce9
@jcbcapps @abbyoung
Update styles
a902eff
@jcbcapps @abbyoung
Move class to module
077e442
@gidjin @abbyoung
fix: resolve type error from useRef return
e403e9c 
See for where the solution came from DefinitelyTyped/DefinitelyTyped#35572 (comment)
@gidjin @abbyoung
test: add article mock to NewsAnnouncements component
b6520be
@abbyoung
fix a11y violation in carousel, update mock in test
444466a
@jcbcapps
Style updates
aa417ab
@jcbcapps
Add test
afd2d18
@jcbcapps
Add test
684bc33
@jcbcapps
Add news page from main
c958f99
@jcbcapps
Add view older internal news link
823e7b4
@jcbcapps
Add comment
9b1914c
@jcbcapps
Update styles
67fd32b
@jcbcapps
Add conditional render for link
e037e7a
@jcbcapps
Merge branch 'main' into sc-589/internal-news-carousel
4de79e5
@jcbcapps
Update text
2f03cc1
@jcbcapps
Display four latest articles
38823fe
@jcbcapps
Update link
b5076e0
@jcbcapps
Update button text
bb4148f
@shortcut-integration
Copy link

This pull request has been linked to:

@shortcut-integration
Copy link

This pull request has been linked to Shortcut Story #772: Finish News & Announcements page.

@jcbcapps jcbcapps requested a review from minhlai as a code owner August 18, 2022 14:26
@jbecker01
Copy link
Contributor

@jcbcapps this all looks good. The only thing I believe that needs to change is the CTA label to "View Details"

gidjin
gidjin approved these changes Aug 22, 2022
View reviewed changes
Copy link
Contributor

@gidjin gidjin left a comment

Choose a reason for hiding this comment

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

This PR is dependent on #744 correct? If so it looks good to me, though I think you may have some updates to apply once #744 is merged. I'm marking it approved assuming that those updates happen :)

Note: I saw John's comment about View Detail but the Figma designs for #744 show View article which is what is on the other branch iirc.

@jbecker01
Copy link
Contributor

@gidjin @jcbcapps you can disregard my last comment on updating the CTA to View Details. I don't think I saw that it was already updated on a different branch.

@jcbcapps jcbcapps merged commit e13b57a into main Aug 23, 2022
@jcbcapps jcbcapps deleted the sc-771/display-latest-articles branch August 23, 2022 19:51
@gidjin gidjin mentioned this pull request Aug 24, 2022
Merged
gidjin added a commit that referenced this pull request Aug 24, 2022
@gidjin
chore(release): 4.7.0 (#758)
3d12c0e 
## [4.7.0](4.6.0...4.7.0) (2022-08-24)


### Features

* Add internal news carousel for news and announcements page ([#744](#744)) ([c4acfe6](c4acfe6)), closes [/github.com/DefinitelyTyped/DefinitelyTyped/issues/35572#issuecomment-498242139](https://github.com/USSF-ORBIT//github.com/DefinitelyTyped/DefinitelyTyped/issues/35572/issues/issuecomment-498242139)
* Display Spaceforce.mil news articles below the news carousel ([#746](#746)) ([e13b57a](e13b57a)), closes [/github.com/DefinitelyTyped/DefinitelyTyped/issues/35572#issuecomment-498242139](https://github.com/USSF-ORBIT//github.com/DefinitelyTyped/DefinitelyTyped/issues/35572/issues/issuecomment-498242139)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gidjin gidjin gidjin approved these changes

@abbyoung abbyoung abbyoung approved these changes

@esacteksab esacteksab Awaiting requested review from esacteksab

@minhlai minhlai Awaiting requested review from minhlai

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@jcbcapps @jbecker01 @gidjin @abbyoung