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-17057 Events detail page web component upgrades #1972

Merged
merged 2 commits into from
Mar 22, 2024

Conversation

randimays
Copy link
Contributor

@randimays randimays commented Mar 18, 2024

Summary

Update individual event template with web components where they exist.

Related issue(s)

Testing done

Tested a couple of events pages locally:

  • /minneapolis-health-care/events/64415/
  • /outreach-and-events/events/53182/

Screenshots

Online event link

Screenshot 2024-03-18 at 2 14 19 PM Screenshot 2024-03-18 at 2 14 13 PM

See more events link

Screenshot 2024-03-18 at 2 14 05 PM Screenshot 2024-03-18 at 2 13 58 PM

Show all times button

Screenshot 2024-03-18 at 2 13 54 PM Screenshot 2024-03-18 at 2 13 49 PM

Add to calendar links inside accordion

Screenshot 2024-03-18 at 2 13 35 PM Screenshot 2024-03-18 at 2 13 30 PM

More times accordion

Screenshot 2024-03-18 at 2 13 23 PM Screenshot 2024-03-18 at 2 13 10 PM

Single add to calendar link

Screenshot 2024-03-18 at 2 12 40 PM Screenshot 2024-03-18 at 2 12 32 PM

Acceptance criteria

  • Links are using V1
  • Accordion is using V3
  • other features display correctly
  • verify/review the E2E tests
  • check desktop and mobile
  • a11y review

Quality Assurance & Testing

  • I fixed|updated|added unit tests and integration tests for each feature (if applicable).
  • Linting warnings have been addressed
  • Screenshot of the developed feature is added
  • Accessibility testing has been performed

@randimays randimays force-pushed the 17057-events-detail-wc branch from 6f4d026 to bb0ee18 Compare March 19, 2024 15:12
@va-vfs-bot va-vfs-bot temporarily deployed to master/main/17057-events-detail-wc March 19, 2024 15:23 Inactive
@randimays randimays marked this pull request as ready for review March 19, 2024 18:29
@randimays randimays requested review from a team as code owners March 19, 2024 18:29
Copy link
Contributor

@eselkin eselkin left a comment

Choose a reason for hiding this comment

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

LGTM

@laflannery
Copy link

@randimays A few comments:

  • I'm comparing an event on the PR environment to the same event on Live
  • The share links are using the <va-link> and are under the "Add to Calendar" Link but I can't see them visually, I can tab to them though.
  • The "Last updated" text and the feedback button isn't on the page anymore. Was this intentional?
  • Because there is only 1 accordion for the recurring events, as we able to remove The "Expand All" option from the component?

@va-vfs-bot va-vfs-bot requested a review from a team March 19, 2024 20:34
Copy link
Collaborator

@va-vfs-bot va-vfs-bot left a comment

Choose a reason for hiding this comment

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

Icon found

Icons can be decorative, but sometimes they are used to convey meaning. If there are any semantics associated with an icon, those semantics should also be conveyed to a screen reader.

What you can do

Review the markup and see if the icon provides information that isn't represented textually, or wait for a VSP review.

<a class="va-js-share-link" href="https://www.facebook.com/sharer/sharer.php?href={{ hostUrl }}{{ entityUrl.path }}">
<i aria-hidden="true" class="va-c-social-icon fab fa-facebook vads-u-margin-right--0p5" role="presentation"></i>Share on Facebook
</a>
<i aria-hidden="true" class="va-c-social-icon fab fa-facebook vads-u-margin-right--0p5" role="presentation"></i>
Copy link
Collaborator

Choose a reason for hiding this comment

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

icon found

<a class="va-js-share-link" href="https://twitter.com/intent/tweet?text={{ title }}&url={{ hostUrl }}{{ entityUrl.path }}">
<i aria-hidden="true" class="va-c-social-icon fab fa-twitter vads-u-margin-right--0p5" role="presentation"></i>Share on Twitter
</a>
<i aria-hidden="true" class="va-c-social-icon fab fa-twitter vads-u-margin-right--0p5" role="presentation"></i>
Copy link
Collaborator

Choose a reason for hiding this comment

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

icon found

@va-vfs-bot va-vfs-bot temporarily deployed to master/main/17057-events-detail-wc March 19, 2024 21:10 Inactive
Copy link
Contributor

@chriskim2311 chriskim2311 left a comment

Choose a reason for hiding this comment

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

Changes look good approved!

@randimays randimays force-pushed the 17057-events-detail-wc branch from 081ee4e to d260a78 Compare March 21, 2024 16:40
@randimays randimays changed the title VACMS-17057 Events web component upgrades VACMS-17057 Events detail page web component upgrades Mar 22, 2024
@randimays randimays merged commit 322bb20 into main Mar 22, 2024
22 checks passed
@randimays randimays deleted the 17057-events-detail-wc branch March 22, 2024 13:37
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.

5 participants