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

Add Hearing icon to queue/case views #8904

Merged
merged 36 commits into from
Feb 6, 2019
Merged

Conversation

joeyyang
Copy link
Contributor

@joeyyang joeyyang commented Jan 23, 2019

Resolves #7478

Description

Adds the HearingBadge "H" to all <TaskTable>s and loads them asynchronously.

ex.
screenshot 2019-01-23 17 31 59
screenshot 2019-01-23 17 32 17

@ghost ghost assigned joeyyang Jan 23, 2019
@ghost ghost added the In-Progress label Jan 23, 2019
@@ -46,7 +46,7 @@ const HearingBadge = ({ hearing }) => {
</div>;

// We expect this badge to be shown in a table, so we use this to get rid of the standard table padding.
return <div {...css({ marginRight: '-3rem' })} className="cf-hearing-badge">
return <div {...css({ marginRight: '-2.5rem' })} className="cf-hearing-badge">
Copy link
Contributor

Choose a reason for hiding this comment

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

Why did we need to make this change?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This spacing works well in all the contexts that the badge shows up in.

Copy link
Contributor

@lowellrex lowellrex left a comment

Choose a reason for hiding this comment

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

In addition to a few small notes I left, let's explore making the request for hearing information for an appeal related to a task asynchronously.

@joeyyang joeyyang changed the title Add Hearing icon to queue/case views [Tech spec] Add Hearing icon to queue/case views Jan 24, 2019
@joeyyang joeyyang changed the title [Tech spec] Add Hearing icon to queue/case views Add Hearing icon to queue/case views Jan 24, 2019
@joeyyang
Copy link
Contributor Author

back to you!

const mapStateToProps = (state, ownProps) => {
let externalId, hearing;

if (ownProps.hearing) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

accept either a hearing or a task

@lowellrex
Copy link
Contributor

Looks good! Can we add a few tests to make sure the hearing icon shows up as we expect (and doesn't) on the case search page and on a queue table view?

@joeyyang
Copy link
Contributor Author

joeyyang commented Feb 1, 2019

Looks good! Can we add a few tests to make sure the hearing icon shows up as we expect (and doesn't) on the case search page and on a queue table view?

Case search hearing badge tests can be found here in search_spec.rb, and I'll add tests for the queue table view now!

@@ -79,7 +79,7 @@

scenario "Entire set of attributes for hearing are displayed" do
visit "/queue"
page.find(:xpath, "//tr[@id='table-row-#{appeal.vacols_id}']/td[1]/a").click
page.find(:xpath, "//tr[@id='table-row-#{appeal.vacols_id}']/td[2]/a").click
Copy link
Contributor

Choose a reason for hiding this comment

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

What do you think about spinning this pattern out to its own helper function? I imagine something like the click_dropdown function perhaps with a signature like find_table_cell(id: appeal.vacols_id, row_header: COPY::CASE_LIST_TABLE_VETERAN_NAME_COLUMN_TITLE).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will make this change in a separate PR!

Copy link
Contributor

@lowellrex lowellrex left a comment

Choose a reason for hiding this comment

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

LGTM!

@joeyyang joeyyang added the Ready-to-Merge This PR is ready to be merged and will be picked up by va-bot to automatically merge to master label Feb 5, 2019
@ghost ghost assigned va-bot Feb 5, 2019
@joeyyang joeyyang removed the Ready-to-Merge This PR is ready to be merged and will be picked up by va-bot to automatically merge to master label Feb 5, 2019
@joeyyang joeyyang added the Ready-to-Merge This PR is ready to be merged and will be picked up by va-bot to automatically merge to master label Feb 6, 2019
@va-bot va-bot merged commit 2aacc22 into master Feb 6, 2019
@va-bot va-bot deleted the joey-hearing-badge-queue branch February 6, 2019 01:25
@ghost ghost removed the In-Progress label Feb 6, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Ready-to-Merge This PR is ready to be merged and will be picked up by va-bot to automatically merge to master
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants