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 17632 no card on non-clinical service locations #2024

Conversation

eselkin
Copy link
Contributor

@eselkin eselkin commented Apr 18, 2024

On Service Location branch - will not pass CI

Summary

  • Non-clinical services currently have a faux-card around them. This removes the borders and x-padding and adds 48px of margin above (there is no 45px margin as in design in VADS, only 40 and 48).
  • Sitewide Facilities

Related issue(s)

Testing done

  • Tested locally on all top-task pages and checked clinical services and VBA
  • Checked that borders and spacing were still the same on all clinical services and VBA

Screenshots

Note: This field is mandatory for UI changes (non-component work should NOT have screenshots).

Old - Bordered - desktop Screenshot 2024-04-18 at 12 01 36 PM Screenshot 2024-04-18 at 12 04 24 PM Screenshot 2024-04-18 at 12 04 50 PM
Old - Bordered - mobile Screenshot 2024-04-18 at 12 03 34 PM
New - No border/x-padding - desktop Screenshot 2024-04-18 at 12 01 22 PM Screenshot 2024-04-18 at 12 04 13 PM Screenshot 2024-04-18 at 12 05 08 PM
New - Non-bordered - mobile Screenshot 2024-04-18 at 12 03 10 PM
Unaffected clinical service Screenshot 2024-04-18 at 12 12 33 PM
Unaffected VBA service location Screenshot 2024-04-18 at 12 12 53 PM

What areas of the site does it impact?

VAMC non-clinical pages

Acceptance criteria

  • Non-clinical service locations do not use the card component/border on these templates
    • Billing and Insurance
    • Medical Records
    • Register for Care
  • Requires design review

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

Review on Service Location branch (do a content release with this PR)
e.g. https://web-xptmfsnjoodldn85nzgkrgljqtfbz24b.ci.cms.va.gov/northeast-ohio-health-care/medical-records-office/#get-your-records-in-person
Make sure to do a full refresh of the page if you've ever visited the page before

@eselkin eselkin marked this pull request as ready for review April 18, 2024 19:22
@eselkin eselkin requested review from a team as code owners April 18, 2024 19:22
@eselkin eselkin requested a review from maxx1128 April 18, 2024 19:22
@va-vfs-bot va-vfs-bot temporarily deployed to master/main/VACMS-17632-Non-Clinical-Services-No-Card April 19, 2024 10:46 Inactive
Copy link

@laflannery laflannery left a comment

Choose a reason for hiding this comment

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

@eselkin I looked at this on the SL Tugboat and there is a lot of whitespace, can we remove all of this:
Screenshot 2024-04-23 at 9 10 54 AM

Then on the other page, referenced in the PR description, it would be better if you removed the custom 0 margin from the H5, this will allow the standard DS spacing to be used:
Screenshot 2024-04-23 at 9 14 02 AM

On the Billing and Insurance pages also, we would want to remove the custom 0 margin on the h4, for the same reason - to allow the standard DS spacing to be used:
Screenshot 2024-04-23 at 9 19 15 AM

Copy link
Contributor

@maxx1128 maxx1128 left a comment

Choose a reason for hiding this comment

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

One small suggestion, once that and Laura's feedback is addressed, I think it'll be good to go.

src/site/paragraphs/service_location.drupal.liquid Outdated Show resolved Hide resolved
@eselkin
Copy link
Contributor Author

eselkin commented Apr 23, 2024

@laflannery @thejordanwood wanted 45px space at the top. There was no 45px so we went with 48px. I guess the space was for some specific reason.

@eselkin
Copy link
Contributor Author

eselkin commented Apr 23, 2024

The spacing on the divs rather than the h4 and 5 was because there were possibly absent headers before. I think. I can remove.

@laflannery
Copy link

@eselkin for our awareness and and future reference, I made a note in the ticket as to why I made my above comment and requested updates and what guidance and files I was following.

@davidmpickett
Copy link
Contributor

To follow up. @laflannery is correct that we should not be adding custom margins here. This should use the standard Design System header margins.

The speculative design file I linked to was only supposed to indicate the lack of cards/borders, not in any other way be guidance for this implementation. My apologies for muddying the waters. The principle we are following for the VBA/VAMC launch is only changing what is absolutely necessary( & a couple carefully targeted enhancements).

@eselkin
Copy link
Contributor Author

eselkin commented Apr 24, 2024

Spacing is updated on SL tugboat @laflannery and @davidmpickett

@davidmpickett
Copy link
Contributor

Looks great!

@laflannery
Copy link

Looks good! And in even better news, after double checking all instances from the mural, I confirmed that this resolved all the remaining funny spacing from the Conditional Location header ticket.

@eselkin eselkin merged commit 57cb55f into VACMS-16144-VAMC-ServiceLocationParagraphs Apr 25, 2024
14 of 19 checks passed
@eselkin eselkin deleted the VACMS-17632-Non-Clinical-Services-No-Card branch April 25, 2024 22:14
eselkin added a commit that referenced this pull request May 14, 2024
* Move to Service Location Paragraphs in VAMC

* merging of service_locations paragraphs

* Service Hours

* remove old service_location paragraphs

* remove consolelog

* typo on include

* conditions when to show Appointments

* show appointments simplification

* test not possible on paragraph

* testing

* naming

* spacing

* indent

* non-clinical

* clena up spacing

* spacing class

* improving spacing slowly

* spacing again

* working spacing

* more spacing

* correct order of phone numbers and h5 for appointment related phone numbers

* order

* borders were not showing

* bordered

* accidental vet center accordion border

* fix headers

* VA-15959: Update VBA Service Location query for int branch (#1936)

* fix for empty extensions

* v3 components added, built, updated

* remove extra vetcenter attributes to liquid

* fix testing on phone numbers now that using va-telephone component

* add more testing

* make use of regexes better

* explanation of matches

* allow for short codes

* additional info uswds

* spacing on Appointments header

* remove Vet Center tests from this branch

* remove Vet Center tests from this branch

* replace should have 2 args

* remove debugging

* remove extra div and strongs

* optional

* VACMS-17513: Remove unspecified referrals from front-end (#1982)

* shows main phone or appt phone numbers and shows main phone or contact phone numbers (#1993)

* Appointments top section visible when walkins unspecified and not remove_text (#2009)

* Appointments top section visible when walkins unspecified
* spacing
* simplify conditions -- no change in logic
* fix? works locally- did not push last time because of new package.json
* update to variables

* not elsif (#2002)

* VACMS 17632 no card on non-clinical service locations  (#2024)

* no card on non-clinical service locations
* remove comment
* updated spacing

* VACMS 17844 update service options values for office visits and virtual support (#2035)

* working values for updated office visits and virtual
* update comment

* VA-17710: Change Online Schedule link for VBA Services (#2026)

* memBefore memAfter

* trying graceful fs

* fix from VACMS-18069-EMFILE-error-memoryUsage

* VA-17598: Manage Benefits online link update (#2057)

* VA-17598: Add missing benefits paragraph (#2078)

* hasAddress default to false

---------

Co-authored-by: Max Antonucci <[email protected]>
Co-authored-by: Max Antonucci <[email protected]>
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