Skip to content
This repository has been archived by the owner on Jan 7, 2020. It is now read-only.

Table heading css #143

Merged
merged 7 commits into from
May 11, 2018
Merged

Table heading css #143

merged 7 commits into from
May 11, 2018

Conversation

awolfe76
Copy link
Contributor

Closes #131

This does some cleanup to improve the heading area throughout the report workflow; for both aggregate and disclosure reports.

  • groups the progress cards a little better, within an <ol>
  • uses the <ol> to give an idea of steps similar to the filing application
  • improves the spacing between each section
  • cleans up the table header section

Screenshot

header-cleanup

@awolfe76 awolfe76 requested a review from wpears May 10, 2018 18:55
headingText="MSA/MD Aggregate Reports"
paragraphText="These reports summarize lending activity by MSA/MD."
/>
)

return (
<>
<React.Fragment>
Copy link
Member

Choose a reason for hiding this comment

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

Any reason for the reintroduction of the longer fragment syntax?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not really ... the <> way causes my editor (Sublime) to do odd things and makes it hard to read.

headingText="Disclosure reports"
paragraphText="These reports summarize lending activity for individual
institutions, both nationwide and by MSA/MD."
/>
)

return this.state.fetched ? (
<>
<React.Fragment>
Copy link
Member

Choose a reason for hiding this comment

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

Another fragment


const capitalize = str => str[0].toUpperCase() + str.slice(1)
const ProgressCard = ({ name, id, link, title }) => {
if (id === 'nationwide') {
name = ''
Copy link
Member

Choose a reason for hiding this comment

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

A nationwide id will still need to be capitalized here, though it could just be hardcoded as id = 'Nationwide'

@wpears
Copy link
Member

wpears commented May 10, 2018

I like the formatting changes , but I think I'd probably prefer to keep the light-gray background on the progress cards. Right now I feel like there isn't enough visual separation between the header, progress cards, and main page element.

@awolfe76
Copy link
Contributor Author

What do you think about adding a border or <hr /> to help with the separation? Adding the background back doesn't work very well with the <ol> when displaying the numbers. The numbers are outside of the background ... and I like the idea of showing steps.

Example with <hr />

with-hr

It might also be helpful if we show all the steps immediately to give someone the full idea of the necessary steps. That would also probably visually help with the separation even more.

Of course we'd have them be active/inactive and updating the content of each according to the current step. For example, if on the choose an institution step the others might say something like "please select an institution first".

Thoughts?

@wpears
Copy link
Member

wpears commented May 10, 2018

I think the hr definitely helps.

Also, I think the idea of having all the steps available could be nice, but we'll just have to make sure the info in the inactive steps isn't too busy. I think a disabled, gray link and a gray, possibly italic "Select an institution" or "Select an Institution and MSA/MD" would do fine.

@awolfe76
Copy link
Contributor Author

@wpears - this is updated with the <hr> and I created #145 to add in the steps all the time. So this should be ready to check again.

@awolfe76 awolfe76 mentioned this pull request May 11, 2018
@wpears wpears merged commit 54c316d into a-and-d May 11, 2018
@wpears wpears deleted the table-heading-css branch May 11, 2018 16:49
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants