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.

Sign up for GitHub

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

Jump to bottom

[Art/96497] - updating POA request list to use cards instead of a table design #33002

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

[Art/96497] - updating POA request list to use cards instead of a table design #33002

wants to merge 10 commits into from
+148 −65

Conversation

jquispe-oddball
Copy link

@jquispe-oddball jquispe-oddball commented Nov 13, 2024 •
edited
Loading

Updating the existing request list to use va-card instead of va-table to match the current figma.

Related ticket

Previous design using va-table:
image

Updated design using va-card:
image

@jquispe-oddball jquispe-oddball added the benefits-accredited-rep-facing Label for the OCTO Slack team #benefits-representative-facing label Nov 13, 2024
@jquispe-oddball jquispe-oddball requested review from a team as code owners November 13, 2024 18:55
@jquispe-oddball jquispe-oddball mentioned this pull request Nov 13, 2024
Open
2 tasks
va-vfs-bot
va-vfs-bot reviewed Nov 13, 2024
View reviewed changes
Copy link

@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.

Note:

Font Awesome is deprecated. Please use va-icon instead. For more information, visit the migration documentation: Migrate from font awesome to va-icon

...plications/accredited-representative-portal/components/POARequestsTable/POARequestsTable.jsx
<div className="poa-requests__col">
<p className="poa-requests__card-label">
{attributes.status === 'Declined' && (
<va-icon
Copy link

Choose a reason for hiding this comment

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

icon found

...plications/accredited-representative-portal/components/POARequestsTable/POARequestsTable.jsx Outdated
class="poa-requests__card-icon--red poa-requests__card-icon"
/>
)}
{attributes.status === 'Accepted' && (
Copy link

Choose a reason for hiding this comment

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

icon found

...plications/accredited-representative-portal/components/POARequestsTable/POARequestsTable.jsx Outdated
</div>

<div className="poa-requests__col">
<p className="poa-requests__card-label">
Copy link

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 reviewed Nov 13, 2024
View reviewed changes
Copy link

@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.

Note:

Font Awesome is deprecated. Please use va-icon instead. For more information, visit the migration documentation: Migrate from font awesome to va-icon

...plications/accredited-representative-portal/components/POARequestsTable/POARequestsTable.jsx
/>
)}
{attributes.status === 'Accepted' && (
<va-icon
Copy link

Choose a reason for hiding this comment

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

icon found

...plications/accredited-representative-portal/components/POARequestsTable/POARequestsTable.jsx

<div className="poa-requests__col">
<p className="poa-requests__card-label">
<va-icon
Copy link

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/art/96497/render-poa-request-list-as-card/main November 13, 2024 19:37 Inactive
@jquispe-oddball jquispe-oddball requested a review from a team as a code owner November 13, 2024 20:06
@va-vfs-bot va-vfs-bot temporarily deployed to master/art/96497/render-poa-request-list-as-card/main November 13, 2024 20:09 Inactive
nihil2501
nihil2501 requested changes Nov 13, 2024
View reviewed changes
Copy link
Contributor

@nihil2501 nihil2501 left a comment •
edited
Loading

Choose a reason for hiding this comment

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

@jquispe-oddball It looks like a change to 2 different applications snuck into this PR.

I will review the change to our application code in a second review I'm conducting now, but figured I'd notify you of this part first.

src/applications/my-education-benefits/config/chapters/33/contactInfo/mailingAddress.js Outdated Show resolved Hide resolved
src/applications/survivor-dependent-education-benefit/22-5490/config/form.js Outdated Show resolved Hide resolved
@jquispe-oddball jquispe-oddball force-pushed the art/96497/render-poa-request-list-as-card branch from b12c760 to 625d017 Compare November 13, 2024 22:02
@va-vfs-bot va-vfs-bot temporarily deployed to master/art/96497/render-poa-request-list-as-card/main November 13, 2024 22:41 Inactive
nihil2501
nihil2501 requested changes Nov 14, 2024
View reviewed changes
Copy link
Contributor

@nihil2501 nihil2501 left a comment

Choose a reason for hiding this comment

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

Excellent.

Requested changes:

  • indentation and new line in scss
  • poaRequest variable name

Nitpick / optional changes:
What do you think of the following suggestions for class names to reflect the hierarchy and significance of elements precisely and consistently?

poa-requests__card-name -> poa-requests__card-title
poa-requests__card-group -> poa-requests__card-subtitle
poa-requests__row -> poa-requests__card-content
poa-requests__col -> poa-requests__card-field
poa-requests__card-label -> poa-requests__card-field-label
poa-requests__card-data -> poa-requests__card-field-value

It doesn't feel like an exact science to me, so I don't feel too strongly either way.

...lications/accredited-representative-portal/components/POARequestsTable/POARequestsTable.scss Outdated Show resolved Hide resolved
...lications/accredited-representative-portal/components/POARequestsTable/POARequestsTable.scss Outdated Show resolved Hide resolved
...plications/accredited-representative-portal/components/POARequestsTable/POARequestsTable.jsx Outdated Show resolved Hide resolved
@jquispe-oddball jquispe-oddball force-pushed the art/96497/render-poa-request-list-as-card branch from 2658ad6 to bef4451 Compare November 14, 2024 15:17
@va-vfs-bot va-vfs-bot temporarily deployed to master/art/96497/render-poa-request-list-as-card/main November 14, 2024 15:17 Inactive
@jquispe-oddball
Copy link
Author

@nihil2501 updated pr based on feedback

@va-vfs-bot va-vfs-bot temporarily deployed to master/art/96497/render-poa-request-list-as-card/main November 14, 2024 15:30 Inactive
@nihil2501
Copy link
Contributor

nihil2501 commented Nov 14, 2024 •
edited
Loading

@nihil2501 updated pr based on feedback

@jquispe-oddball For the variable name update, poaRequests was named correctly. I was suggesting updating the name of the variable for mapping over that collection from attributes to poaRequest so it reads like poaRequests.map({ id, poaRequest }) => { ... })

@va-vfs-bot va-vfs-bot temporarily deployed to master/art/96497/render-poa-request-list-as-card/main November 14, 2024 16:58 Inactive
@nihil2501 nihil2501 self-requested a review November 14, 2024 18:04
nihil2501
nihil2501 approved these changes Nov 14, 2024
View reviewed changes
Copy link
Contributor

@nihil2501 nihil2501 left a comment

Choose a reason for hiding this comment

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

LGTM

@nihil2501 nihil2501 self-requested a review November 14, 2024 18:05
nihil2501
nihil2501 requested changes Nov 14, 2024
View reviewed changes
Copy link
Contributor

@nihil2501 nihil2501 left a comment

Choose a reason for hiding this comment

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

@jquispe-oddball It looks like unit tests are failing due to a syntax error somewhere

@jquispe-oddball jquispe-oddball force-pushed the art/96497/render-poa-request-list-as-card branch from 74621a8 to 2cd8c1b Compare November 14, 2024 19:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@va-vfs-bot va-vfs-bot va-vfs-bot left review comments

@chumpy chumpy chumpy left review comments

@nihil2501 nihil2501 nihil2501 requested changes

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
benefits-accredited-rep-facing Label for the OCTO Slack team #benefits-representative-facing
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@jquispe-oddball @nihil2501 @chumpy @va-vfs-bot