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

VACMS 18724 - Adds mission explainer to Vet Centers #2307

Merged
merged 3 commits into from
Oct 29, 2024

Conversation

eselkin
Copy link
Contributor

@eselkin eselkin commented Oct 8, 2024

DO NOT MERGE UNTIL NOTIFICATIONS MADE TO VET CENTER EDITORS

Summary

  • Adds mission explainer query to graphQL and pulls it into the layout for Vet Centers.
  • Sitewide team (owns Vet Center facility pages and queries)

Related issue(s)

Testing done

  • No mission explainer prior
  • Verify mission explainer exists on Vet Center pages that have the centralized content (in some tugboats there may be some that do not have it, but in prod and staging all do now)

Screenshots

Vet Center Page with mission explainer (Boston Vet Center) - Desktop (above 768px) Screenshot 2024-10-10 at 2 43 24 PM
Vet Center Page with mission explainer (Boston Vet Center) - below 768px Screenshot 2024-10-10 at 2 45 34 PM
Vet Center Page missing mission explainer in tugboat (looks exactly the same as current Vet Center) - fixed in prod and staging so this does not occur but added to show logic exists if some VC removes it somehow Screenshot 2024-10-07 at 10 12 12 PM

What areas of the site does it impact?

Affects Vet Center pages

Acceptance criteria

Quality Assurance & Testing

  • I fixed|updated|added unit tests and integration tests for each feature (if applicable).
  • No sensitive information (i.e. PII/credentials/internal URLs/etc.) is captured in logging, hardcoded, or specs
  • Linting warnings have been addressed
  • Documentation has been updated (link to documentation *if necessary)
  • Screenshot of the developed feature is added
  • Accessibility testing has been performed

Error Handling

  • Browser console contains no warnings or errors.
  • Events are being sent to the appropriate logging solution
  • Feature/bug has a monitor built into Datadog or Grafana (if applicable)

Authentication

  • Did you login to a local build and verify all authenticated routes work as expected with a test user

Requested Feedback

Check https://web-rdikukigkjapf0z74ocsofi8izmc2gmo.demo.cms.va.gov/boston-vet-center/
If this is reset before you view, re-release content on that tugboat with 2307 as the PR for content-build.

Check page/component at 767px or lower and 769px or greater. At 767px or lower, should have a margin of 32px below mission explainer and above va-on-this-page component (with a border). At 769px or greater, should not have this margin, because the lack of border makes it appear far enough away from the va-on-this-page content

@eselkin
Copy link
Contributor Author

eselkin commented Oct 8, 2024

The RI doesn't have the change or the Vet Center on the RI CMS doesn't have the centralized content.

@eselkin eselkin marked this pull request as ready for review October 8, 2024 22:59
@eselkin eselkin requested review from a team as code owners October 8, 2024 22:59
@randimays
Copy link
Contributor

We may want some vertical space between the mission explainer box and the "On this page" component since the OTP component has a border on mobile. @thejordanwood

Screenshot 2024-10-10 at 5 58 40 AM

@thejordanwood
Copy link

@randimays I agree. Can we get 32px (vads-spacing-4) between the mission explainer and the "On this page" component on mobile?

@eselkin
Copy link
Contributor Author

eselkin commented Oct 10, 2024

Yep, probably a margin of that size on the bottom. will add. Just on mobile only though? @thejordanwood

@thejordanwood
Copy link

@eselkin Yes, just for mobile. I think desktop looks fine.

@eselkin
Copy link
Contributor Author

eselkin commented Oct 10, 2024

Since the box and no margin starts beneath 768px (medium-screen), I made the margin 4 (32px) below medium screen, and 0 otherwise. @randimays after it builds again I'll ping you (probably tomorrow).

@va-vfs-bot va-vfs-bot temporarily deployed to master/main/VACMS-18724-mission-explainer-vet-center October 10, 2024 22:03 Inactive
Copy link
Contributor

@randimays randimays left a comment

Choose a reason for hiding this comment

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

LGTM

@jilladams
Copy link
Contributor

@laflannery @thejordanwood flagging: this ticket has Design / A11y review in ACs, and I'm not sure if you've seen it yet. It exists if you'd like to take a look, though.

@jilladams
Copy link
Contributor

@laflannery @thejordanwood flagging: this ticket has Design / A11y review in ACs, and I'm not sure if you've seen it yet. It exists if you'd like to take a look, though.

JK, the RI is down. Ugh. We'll have to fix that up first.

@randimays
Copy link
Contributor

I re-ran the Jenkins job which will hopefully get the RI back up and running (maybe in 15-20 mins? - honestly not sure how long it takes).

@eselkin
Copy link
Contributor Author

eselkin commented Oct 15, 2024

I think it takes more than 40 minutes for RI to spin up usually.

@eselkin
Copy link
Contributor Author

eselkin commented Oct 15, 2024

@laflannery
Copy link

LGTM!

@thejordanwood
Copy link

Looks good to me!

@eselkin eselkin merged commit 45052b3 into main Oct 29, 2024
24 checks passed
@eselkin eselkin deleted the VACMS-18724-mission-explainer-vet-center branch October 29, 2024 17:40
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.

6 participants