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

Remove sortable table #14966

Merged
merged 12 commits into from Nov 20, 2020
Merged

Remove sortable table #14966

merged 12 commits into from Nov 20, 2020

Conversation

bkjohnson
Copy link
Contributor

@bkjohnson bkjohnson commented Nov 14, 2020

Description

This replaces the one instance of <SortableTable> with the newly-created responsive <Table>. This will allow <SortableTable> to be removed from the component library.

Part of department-of-veterans-affairs/va.gov-team#16429

Testing done

Manual visual testing in browser

Screenshots

Current - Desktop

image

Current - Mobile

image


After - Desktop

image

After - Mobile

image

Acceptance criteria

  • [ ]

Definition of done

  • Events are logged appropriately
  • Documentation has been updated, if applicable
  • A link has been provided to the originating GitHub issue (or connected to it via ZenHub)
  • No sensitive information (i.e. PII/credentials/internal URLs/etc.) is captured in logging, hardcoded, or specs

@va-vfs-bot va-vfs-bot requested a review from a team November 14, 2020 01:37
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.


const attachmentIcon = message.attachment ? (
<i className="fa fa-paperclip" aria-label="Message has an attachment" />
<i
className="fa fa-paperclip vads-u-flex--auto"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

vads-u-flex--auto is to shift this icon all the way to the right, since it is now included with the Subject column instead of its own blank one.

Comment on lines +65 to +72
senderName: message.senderName,
subject: (
<div className="vads-u-display--flex">
<span className="vads-u-flex--fill">{message.subject}</span>
{attachmentIcon}
</div>
),
sentDate: formattedDate(message.sentDate),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

  • hasAttachment is no longer needed since we removed the object with that value from the fields prop.
  • id and rowClass were specific to <SortableTable>
  • We don't need to call makeMessageLink with the new <Table> - all it was doing was outputting content in an empty <a> tag, which looked weird and was useless without the href


return {
attributes,
loading: msgState.loading,
messages,
recipients: msgState.recipients.data,
sort,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The sort prop was only used to pass to <SortableTable>'s currentSort prop. <Table> won't break if currentSort is absent, and the table is configured so that it isn't sortable anyway.

@@ -14,6 +14,7 @@
@import "~@department-of-veterans-affairs/formation/sass/modules/m-additional-info";
@import "~@department-of-veterans-affairs/formation/sass/modules/m-hub-page-link-list";
@import "~@department-of-veterans-affairs/formation/sass/modules/m-promo-banner";
@import "~@department-of-veterans-affairs/formation/sass/modules/m-responsive-tables";
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Need to make sure we get the CSS for responsive tables into vets-website.

@va-vfs-bot va-vfs-bot temporarily deployed to master/removeSortableTable November 16, 2020 19:47 Inactive
@bkjohnson bkjohnson marked this pull request as ready for review November 16, 2020 20:08
@bkjohnson bkjohnson requested review from a team as code owners November 16, 2020 20:08
@va-vfs-bot va-vfs-bot temporarily deployed to master/removeSortableTable November 17, 2020 23:15 Inactive
Copy link
Contributor

@erikphansen erikphansen left a comment

Choose a reason for hiding this comment

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

@tressaellen @callen2563 @Samara-Strauss @mshea0606 should all be made aware of this visual change.

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.

None yet

6 participants