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

VAMS-20163 news release contact number #919

Open
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

brianseek
Copy link
Contributor

@brianseek brianseek commented Mar 4, 2025

Description

Adds media contact phone number to press releases. Adds query into paragraph entity for phone. Adds the va-telephone webcomponent into IntrinsicElements. Changes the person_profile to use the phone type paragraph for phone instead of the incorrect phone string, this created changes beyond press release. Updates mocks to return more data for contacts and phone.

Generated description

This pull request includes several changes to the handling and formatting of contact information, particularly phone numbers, in the pressRelease and newsStory data queries. The changes ensure that phone numbers are now treated as arrays of objects, allowing for multiple numbers and additional details such as type and extension.

Changes to contact information handling:

Changes to tests and components:

Ticket

closes #20163.

Developer Task

  • PR submitted against the main branch of next-build.
  • Link to the issue that this PR addresses (if applicable).
  • Define all changes in your PR and note any changes that could potentially be breaking changes.
  • PR includes steps to test your changes and links to these changes in the Tugboat preview (if applicable).
  • Provided before and after screenshots of your changes (if applicable).
  • Alerted the #accelerated-publishing Slack channel to request a PR review.
  • You understand that once approved, you are responsible for merging your changes into main. (Note that changes to main will move automatically into production.)

Testing Steps

Screenshots

Screenshot 2025-03-06 at 12 33 42 PM

Reviewer

Reviewing a PR

This section lists items that need to be checked or updated when making changes to this repository.

Standard Checks

  • Code Quality: Readabilty, Naming Conventions, Consistency, Reusability
  • Test Coverage: 80% coverage
  • Functionality: Change functions as expected with no additional bugs
  • Performance: Code does not introduce performance issues
  • Documentation: Changes are documented in their respective README.md files
  • Security: Packages have been approved in the TRM

Merging an Approved Layout

When merging a layout, you must ensure that the content type has been turned on for next-build inside the CMS. This CMS flag must be turned on for editors to preview their work using the next build preview server.

Resource types (layouts) that have not been approved by design should NOT be pushed to production. Ensure that slug.tsx does not include your resource type if it is not approved.

The status of layouts should be kept up to date inside templates.md. This includes QA progress, development progress, etc. A link should be provided for where testing can occur.

Merging a Non-Approved Layout

Your new resource type should not be included inside slug.tsx. Items added here will go into production once merged into the main branch. It is imperative that we do not push anything live that has not been approved.

Ensure that this layout has been added to the templates.md file with the current status of the work.

@va-cms-bot va-cms-bot temporarily deployed to Tugboat March 4, 2025 18:04 Destroyed
@va-cms-bot va-cms-bot temporarily deployed to Tugboat March 4, 2025 19:05 Destroyed
@va-cms-bot va-cms-bot temporarily deployed to Tugboat March 4, 2025 19:08 Destroyed
@brianseek brianseek force-pushed the VACMS-20163-news-release-contact-number branch from dd61b73 to 5baaf95 Compare March 5, 2025 22:14
@va-cms-bot va-cms-bot temporarily deployed to Tugboat March 5, 2025 22:14 Destroyed
@va-cms-bot va-cms-bot temporarily deployed to Tugboat March 5, 2025 23:01 Destroyed
@va-cms-bot va-cms-bot temporarily deployed to Tugboat March 5, 2025 23:06 Destroyed
@va-cms-bot va-cms-bot temporarily deployed to Tugboat March 5, 2025 23:09 Destroyed
@va-cms-bot va-cms-bot temporarily deployed to Tugboat March 6, 2025 17:41 Destroyed
@brianseek brianseek marked this pull request as ready for review March 6, 2025 20:34
@brianseek brianseek requested review from a team as code owners March 6, 2025 20:34
@laflannery
Copy link

Looks good - I Tested standard phone numbers and exts. I don't currently know of any instances where Fax, TTY or SMS are used here so I wasn't able to confirm those

Comment on lines +89 to +92
id: number?.id || null,
type: number?.field_phone_number_type || null,
number: number?.field_phone_number || null,
ext: number?.field_phone_extension || null,
Copy link
Contributor

Choose a reason for hiding this comment

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

😅 What happens if these are false-y? I can't think of a reason we'd ever want empty strings for any of these, but this will turn those empty strings into null. 🤔 Probably fine? 🤷

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was also dubious about this, but was trying to follow style/pattern. Getting existential about the identity of a defect ticket, it was feeling weird to shift that style for this work. Mostly because I would then feel the need to change other un-related areas to match. I think next steps around this sort of thing for me at least would be to hook my local drupal up to local next to start playing around with different missing data states.

cvalarida
cvalarida previously approved these changes Mar 7, 2025
Copy link
Contributor

@cvalarida cvalarida left a comment

Choose a reason for hiding this comment

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

I'd like to see a few things up, but I think it looks generally good.

@va-cms-bot va-cms-bot temporarily deployed to Tugboat March 7, 2025 16:16 Destroyed
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