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

Disclosure s3 server and select component #27

Merged
merged 10 commits into from
Apr 17, 2018
Merged

Disclosure s3 server and select component #27

merged 10 commits into from
Apr 17, 2018

Conversation

wpears
Copy link
Member

@wpears wpears commented Apr 13, 2018

Also against a-and d, built the work fleshing out the aggregate stuff (though that's broken now)
👀

@wpears wpears requested a review from awolfe76 April 13, 2018 21:47
Copy link
Contributor

@awolfe76 awolfe76 left a comment

Choose a reason for hiding this comment

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

Just a few things.

@@ -59,7 +60,7 @@
"eslint-config-react-app": "2.1.0",
"eslint-plugin-flowtype": "2.44.0",
"eslint-plugin-import": "2.8.0",
"eslint-plugin-jsx-a11y": "6.0.3",
"eslint-plugin-jsx-a11y": "5.1.1",
Copy link
Contributor

Choose a reason for hiding this comment

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

Why back to a previous version?

Copy link
Member Author

Choose a reason for hiding this comment

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

eslint-config-react-app needs a 5.x version to work in all cases, my editor was showing a failure due to conflict on every file.

if (this.state.report === null) return null

const report = this.state.report
const headingText = report
? `Table ${report.table}: ${report.description}, ${report.year}`
? `Table ${this.props.match.params.reportId.split('.txt')[0]}: ${
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should just use the report.table value here, since that data is loaded.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ahh cool, updated.

}
import Selector from './Selector.jsx'

const Reports = props => {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this is ok for now, but we'll end up just having these values all listed, a constant.

There won't be an endpoint since all of the reports were created, even if they were filled with 0.

Copy link
Member Author

Choose a reason for hiding this comment

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

Noted in #47

@awolfe76 awolfe76 merged commit ff9674c into a-and-d Apr 17, 2018
@awolfe76 awolfe76 deleted the alt-disclosure branch April 17, 2018 15:40
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