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: Enable dark mode toggle #859

Merged
merged 16 commits into from
Nov 16, 2022
Merged

Conversation

gidjin
Copy link
Contributor

@gidjin gidjin commented Nov 8, 2022

SC-1248

Proposed changes

This PR replaces #839 and merges in the developer work flow improvements. It also enables the

  • Fix sysinfo so that git sha aka BUILD_ID is displayed
  • Add way to run portal in docker mimicking deployed environments
  • Minor improvements to our docker compose configuration
    • These are also the reason this USSF-ORBIT/ussf-portal-cms#204 was created as well
  • Enable Theme Toggle on all pages
  • Remove Launch Darkly configuration
  • Fix CODEOWNERS configuration

Reviewer notes

  • Make sure ThemeToggle button is working via normal development process (aka yarn dev)
  • Make sure everything appears to work in docker container. Run yarn portal:up See also updated docs in development.md file
  • Review changes in development.md

Reviewer notes

Per our discussions we are aiming to release DarkMode at the end of the iteration. This pr does that and removes the launch darkly stuff.

Setup

Typical yarn services:up and yarn dev in cms and client repos

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

@gidjin gidjin requested a review from a team as a code owner November 8, 2022 23:39
@shortcut-integration
Copy link

This pull request has been linked to Shortcut Story #1248: Launch darkly client id is not loading at run time.

@gidjin gidjin force-pushed the sc-1248/development-flow-improvements branch from 3043336 to f8c9e16 Compare November 8, 2022 23:40
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.

Only looked at the visuals (toggle theme on all pages) but looks good to me!

@gidjin gidjin force-pushed the sc-1248/development-flow-improvements branch from 6bdb73e to 4963a24 Compare November 15, 2022 19:32
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! I left one question, but I don't think it's blocking. I also ran e2e tests locally and they were all passing for me, so not sure why they keep failing here.

@@ -4,6 +4,9 @@ services:
build:
context: https://github.com/USSF-ORBIT/ussf-portal-cms.git#main
dockerfile: Dockerfile
target: e2e-local
Copy link
Contributor

Choose a reason for hiding this comment

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

Does 'e2e-local' need to be hard coded here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good question. I think it's okay here since we only ever use the docker-compose.cms.yml when running things locally in docker via yarn cms:up. If we started using this docker compose file elsewhere we would want to consider changing it like in USSF-ORBIT/ussf-portal#163. That PR is different since the E2E workflow uses docker compose to start up all the various services.

@gidjin gidjin merged commit a3a02a1 into main Nov 16, 2022
@gidjin gidjin deleted the sc-1248/development-flow-improvements branch November 16, 2022 23:07
@gidjin gidjin mentioned this pull request Nov 17, 2022
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