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: Create USSF Documentation page #860

Merged
merged 21 commits into from
Nov 18, 2022
Merged

feat: Create USSF Documentation page #860

merged 21 commits into from
Nov 18, 2022

Conversation

jcbcapps
Copy link
Contributor

@jcbcapps jcbcapps commented Nov 10, 2022 •
edited by malworks
Loading

SC-543

Proposed changes

  • Adds new page /ussf-documentation
  • Moves Essential Reading docs from /about-us to /ussf-documentation
  • Updates <EPubsCard /> component to make the query prop optional and adds an optional title prop
  • Also updated e2e tests here

Reviewer notes

  • Verify that 'USSF documentation' is now an option in the side navigation
  • Navigate to /ussf-documentation and verify that it matches the below screenshot
  • Toggle the Accordion component and verify that the 'Essential Reading' docs are present
  • Click the link in the ePubs box and verify that it takes you to the ePubs search page
  • Toggle light/dark mode and verify that the styles match the design in Figma

Setup

As always, run both the portal client and the CMS on your local machine

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

docs-page

@shortcut-integration
Copy link

This pull request has been linked to Shortcut Story #543: Create Documents page.

@jcbcapps jcbcapps changed the title Sc 543/documents page feat: Create USSF Documentation page Nov 10, 2022
@jcbcapps jcbcapps marked this pull request as ready for review November 14, 2022 23:00
malworks
malworks suggested changes Nov 16, 2022
View reviewed changes
Copy link
Contributor

@malworks malworks left a comment

Choose a reason for hiding this comment

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

Hi! Looks great in light and dark mode. A couple things:

  • The accordion headers ("Essential Reading") are using h3's, when they should be using the label tag.

image

  • Question: The design breaks at more narrow tablet/mobile widths but I'm not sure we're "officially" supporting mobile or not. Are we?
  • Figma has the document type icon listed in front of each document. Is this in scope? (See photo below)

image

gidjin
gidjin approved these changes Nov 16, 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.

Minor note about the test structure in the new test file, but I don't see a need to block the PR on that. Approving from an eng stand point, please follow up with Mallory on the notes she left.

Question: Should the Essential Reading section be expanded by default since it's the only one? Might be a good question for @malworks or @shkeating

src/__tests__/pages/ussf-documentation.test.tsx Outdated
Comment on lines 58 to 64
beforeEach(() => {
html = renderWithAuth(
<MockedProvider>
<USSFDocumentation />
</MockedProvider>
)
})
Copy link
Contributor

Choose a reason for hiding this comment

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

Thought, is there a reason to continue this pattern of sharing html. IIRC the recommendation is to not do that now with react-testing-library. I think this was the article: https://kentcdodds.com/blog/avoid-nesting-when-youre-testing

Suggestion: refactor this to call render in each it block or just combine them all into one test. I can't find the article that recommended that but I read one from Kent Dodds about that.

@jcbcapps
Copy link
Contributor Author

Hi! Looks great in light and dark mode. A couple things:

  • The accordion headers ("Essential Reading") are using h3's, when they should be using the label tag.

  • Question: The design breaks at more narrow tablet/mobile widths but I'm not sure we're "officially" supporting mobile or not. Are we?

  • Figma has the document type icon listed in front of each document. Is this in scope? (See photo below)

@malworks

  1. Good catch on the h3's, I'll make that change.
  2. We don't currently support mobile and, as far as I know, we have no concrete plans to do so.
  3. I skipped this for now because we're still working on the file upload functionality. I plan to write a chore in Shortcut to loop back and move these files to the CMS after we get file upload done.

@jcbcapps
Copy link
Contributor Author

@malworks I spoke too soon on changing the h3 in the Accordion. Guess I should have mentioned that the Accordion component is a ReactUSWDS component, and it uses a headingLevel prop for each item that is required. So I can't make that change, but it doesn't seem crucial to me. Any thoughts?

Also, @gidjin had a great question above asking if the 'Essential Reading' section should be open by default since it is the only Accordion item. I went ahead and made that change so that you could see what it would look like, but let me know if I need to change it back!

@jcbcapps jcbcapps requested a review from malworks November 18, 2022 15:02
@malworks
Copy link
Contributor

@malworks I spoke too soon on changing the h3 in the Accordion. Guess I should have mentioned that the Accordion component is a ReactUSWDS component, and it uses a headingLevel prop for each item that is required. So I can't make that change, but it doesn't seem crucial to me. Any thoughts?

Also, @gidjin had a great question above asking if the 'Essential Reading' section should be open by default since it is the only Accordion item. I went ahead and made that change so that you could see what it would look like, but let me know if I need to change it back!

No worries, it's fine as is. And nice addition to keep the 'Essential Reading' open by default!

@jcbcapps jcbcapps merged commit 6c5d6b3 into main Nov 18, 2022
@jcbcapps jcbcapps deleted the sc-543/documents-page branch November 18, 2022 20:00
@gidjin gidjin mentioned this pull request Nov 30, 2022
Merged
gidjin added a commit that referenced this pull request Nov 30, 2022
@gidjin
chore(release): 4.11.0 (#875)
a137131 
## [4.11.0](4.10.0...4.11.0) (2022-11-30)


### Features

* Create USSF Documentation page ([#860](#860)) ([6c5d6b3](6c5d6b3))
* UI modifications to the article template ([#856](#856)) ([dc7402c](dc7402c))
@gidjin gidjin mentioned this pull request Dec 13, 2022
Merged
gidjin added a commit that referenced this pull request Dec 13, 2022
@gidjin
chore(release): 4.12.0 (#890)
026af9c 
## [4.12.0](4.10.0...4.12.0) (2022-12-13)


### Features

* Add support for hero image in articles ([#862](#862)) ([56721db](56721db))
* add USSF Guardian Commitment Poster to docs page ([#888](#888)) ([07fb472](07fb472))
* Create USSF Documentation page ([#860](#860)) ([6c5d6b3](6c5d6b3))
* Display hero image in article layout ([#884](#884)) ([b2201ac](b2201ac))
* UI modifications to the article template ([#856](#856)) ([dc7402c](dc7402c))


### Bug Fixes

* **deps:** update dependency axios to v1 ([#851](#851)) ([de30320](de30320))


### Security Improvements

* **deps:** update node.js to v14.21.1 ([#849](#849)) ([46aba91](46aba91))
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

@malworks malworks malworks approved these changes

@abbyoung abbyoung Awaiting requested review from abbyoung

@esacteksab esacteksab Awaiting requested review from esacteksab

@minhlai minhlai Awaiting requested review from minhlai

@shkeating shkeating Awaiting requested review from shkeating

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

Successfully merging this pull request may close these issues.
3 participants
@jcbcapps @malworks @gidjin