-
Notifications
You must be signed in to change notification settings - Fork 70
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-17177: Shows descriptions for vet center services on node view/edit #17267
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks very clean and easy to read.
I had one very minor suggestion.
|| ($event->getDisplay()->getOriginalMode() !== 'full')) { | ||
return; | ||
} | ||
$build = &$event->getBuild(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Question (for my own edification): Why does this need to be called via reference?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm pretty sure that the reason for this is because we are making changes to the build. If we didn't call it by reference, then we'd just be getting a copy of the build and any changes we make to it below (line 178) wouldn't be reflected in the actual build.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense.
docroot/modules/custom/va_gov_facilities/src/EventSubscriber/FacilitiesSubscriber.php
Outdated
Show resolved
Hide resolved
Co-authored-by: Christian Burk <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ready to go! Nice work.
Description
Relates to #17177.
Testing done
Manual testing
Screenshots
QA steps
What needs to be checked to prove this works?
What needs to be checked to prove it didn't break any related things?
What variations of circumstances (users, actions, values) need to be checked?
As the QA Vet Center Editor user
Definition of Done
Select Team for PR review
CMS Team
Public websites
Facilities
User support
Accelerated Publishing
Is this PR blocked by another PR?
DO NOT MERGE
Does this PR need review from a Product Owner
Needs PO review
CMS user-facing announcement
Is an announcement needed to let editors know of this change?