Skip to content
This repository has been archived by the owner on Dec 23, 2017. It is now read-only.

Feature/committee file date #312

Merged
merged 6 commits into from
Jul 15, 2015

Conversation

jmcarp
Copy link
Contributor

@jmcarp jmcarp commented Jul 2, 2015

Add file date filter and column.

[Resolves #289]

This is working as expected, but probably needs more discussion; see #289.

@jmcarp jmcarp force-pushed the feature/committee-file-date branch from 7c80af2 to 0ac41a7 Compare July 2, 2015 15:50
Note: I'm keeping lots of the utility code that was used for this
filter, since we're going to be using it for other views.
@jmcarp
Copy link
Contributor Author

jmcarp commented Jul 2, 2015

This has been updated per Slack / GitHub discussion @noahmanger @LindsayYoung.

jmcarp added a commit to jmcarp/openFEC-web-app that referenced this pull request Jul 7, 2015
Borrowed from defunct code in fecgov#312.
@jmcarp
Copy link
Contributor Author

jmcarp commented Jul 7, 2015

Pinging @noahmanger @LindsayYoung for review. After we talked this through last week, the patch became very simple and should be quick to review--it basically just adds a sortable column on first file date, and a date formatter helper for the frontend. There's also some utility code for calculating the current month, quarter, and year, which isn't used here but will be in #302.

@noahmanger
Copy link
Contributor

Looks good, but the problem is that there's no way to see the most recent because there's a bunch of committees with blanks in that column. How should we handle this?

@jmcarp
Copy link
Contributor Author

jmcarp commented Jul 7, 2015

We could change the sorting logic to filter out NULL values--if you're sorting on date, whether ascending or descending, you probably only want to see records that have a date.

@jmcarp
Copy link
Contributor Author

jmcarp commented Jul 7, 2015

We could also have the database treat NULL values as small (by default, they're considered large). That would help with finding the most recent records, but then looking for the least recent would return a bunch of records with NULL dates.

@jmcarp
Copy link
Contributor Author

jmcarp commented Jul 7, 2015

Any preferences @noahmanger @LindsayYoung? I think omitting records with null values on the sorted column is reasonable, but maybe there are better solutions.

jmcarp added a commit to jmcarp/openFEC-web-app that referenced this pull request Jul 9, 2015
@jmcarp
Copy link
Contributor Author

jmcarp commented Jul 9, 2015

@noahmanger the point you brought up about empty rows will be resolved once #322 is merged.

@msecret
Copy link
Contributor

msecret commented Jul 10, 2015

There's quite a few JS libs added in this code.

I know I hadn't completely set this up yet before I moved to less time on this project, but ideally, when theres JS libs added, I'd like to see what the impact would be. Like how many kbs added, and how that effects things like time to first paint, speed index, or custom metrics. Although I understand this is hard to do now, just wanted to comment as a good place to move to.

@noahmanger
Copy link
Contributor

Good looking out @msecret . What do you all think about hiding the "Treasurer Name" column? @LindsayYoung I think this was a suggestion you made a while back?

@jmcarp
Copy link
Contributor Author

jmcarp commented Jul 13, 2015

The libraries added here should each be < 30 kB, minified and gzipped. If we're looking to optimize load times, I'm guessing we'd get the most traction from splitting the browserify bundle into pieces--loading d3, handlebars, and datatables on the homepage (which uses none of the above).

@noahmanger
Copy link
Contributor

Should we make a separate issue for looking at performance and making these sorts of changes?

@jmcarp
Copy link
Contributor Author

jmcarp commented Jul 13, 2015

We have an issue for splitting code bundles: https://github.com/18F/openFEC/issues/908. We also have performance tracking on dev, which we should be able to track with New Relic (is that right @msecret?)--maybe we need an issue for gathering / evaluating those performance numbers.

@msecret
Copy link
Contributor

msecret commented Jul 13, 2015

I'd like to check out New Relic to see what performance tracking they have implemented.

So the thing with performance that I've gathered: the best metric to really get a sense of how your users are experiencing the site is SpeedIndex. It's also the hardest to collect, essentially requiring a webpagetest.org instance running to capture a video of the site loading. This means that often an easier to measure metric, that also comes closer emulates the SpeedIndex metric is a custom metric. For us on FEC, this would be different on each view/page, and could be things like, "time till the whole results table shows", "time till search bar shows", "time till search results appear", etc. I think defining these custom metrics and coming up with what our goal for them would be a good step forward for tracking performance.

@LindsayYoung
Copy link
Contributor

I have mixed feelings on Treasurer name- it is very important since these are the people legally responsible, but it is not necessary everywhere.

@noahmanger
Copy link
Contributor

Yeah, I share that. If we're going to keep it we should also have a filter
for it, which I just realized we're missing. Maybe we just leave it in for
now.

On Mon, Jul 13, 2015 at 10:58 AM, Lindsay Young notifications@github.com
wrote:

I have mixed feelings on Treasurer name- it is very important since these
are the people legally responsible, but it is not necessary everywhere.


Reply to this email directly or view it on GitHub
#312 (comment).

Noah Manger
18F http://18f.gsa.gov | Work: 415-696-6146

jmcarp added a commit to jmcarp/openFEC-web-app that referenced this pull request Jul 13, 2015
@noahmanger
Copy link
Contributor

This looks good to me, but @msecret should probably review the js / python if you haven't already.

@jmcarp
Copy link
Contributor Author

jmcarp commented Jul 15, 2015

This should be good to merge @msecret / @noahmanger --it's just adding a column and updating some tests.

msecret pushed a commit that referenced this pull request Jul 15, 2015
@msecret msecret merged commit c614a80 into fecgov:develop Jul 15, 2015
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants