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 summary row for table #646

Merged
merged 2 commits into from Sep 18, 2017
Merged

Conversation

brittainhard
Copy link
Contributor

@GoFroggyRun this is a dirty prototype but it works. It just loops through the tables of the existing data structure and adds up their values, then creates a new column. The front-end is handling it well, no changes need to be made there. I haven't tested this solution thoroughly, and the code could be improved. This is a good basis for the feature I think. My output from a reform looks like this:

screen shot 2017-09-01 at 1 44 29 pm

It's adding the column AFTER the table has already been saved in the database. I think this can also be done after before the data is saved in the database. I don't think that will cause issues with backwards compatibility since the javascript table is just accepting arbitrary data as long as its in the right format.

@martinholmer @hdoupe your input on this would he helpful I think.

@martinholmer
Copy link
Contributor

@brittainhard, I think it would be far better to label the new column Total (instead of Summary).

@hdoupe
Copy link
Collaborator

hdoupe commented Sep 1, 2017

@brittainhard said

It's adding the column AFTER the table has already been saved in the database. I think this can also be done after before the data is saved in the database. I don't think that will cause issues with backwards compatibility since the javascript table is just accepting arbitrary data as long as its in the right format.

I think that this should probably be done before the model is saved to the database. That would make more sense to me. However, I am not very familiar with java script. So I have no comments on the backwards compatibility of this.

@martinholmer
Copy link
Contributor

@brittainhard, Thanks for the quick response.

@brittainhard
Copy link
Contributor Author

@hdoupe that makes sense, but if we were to need to make changes to the column, or even add another column, the older reforms wouldn't have this up-to-date structure.

@GoFroggyRun
Copy link
Contributor

@brittainhard It seems to me that having this number after the database shouldn't be a problem. Javascript should be able to take care it.

Thanks for taking care of this enhancement. I will work on some refactorizations after this PR being merged.

@brittainhard
Copy link
Contributor Author

@GoFroggyRun I don't think you're going to have to change any of the javascript. That's the nice part of this approach. I want to change the style of the code I wrote and probably refactor it, but then I think it will solve the issue.

Is this a high priority issue?

@martinholmer
Copy link
Contributor

martinholmer commented Sep 1, 2017

@brittainhard asked in #646:

Is this a high priority issue?

Yes! This enhancement request dates back to May 3, 2017 (see issue #545).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants