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

Rename macro elasticity label and remove dropdown menu in its result page #684

Merged
merged 4 commits into from
Oct 26, 2017

Conversation

GoFroggyRun
Copy link
Contributor

This PR fixes #673 and #683. The new page/button would look something like:

screen shot 2017-10-05 at 10 54 29 am

and

screen shot 2017-10-05 at 10 54 23 am

and finally

screen shot 2017-10-05 at 10 54 04 am

@martinholmer
Copy link
Contributor

@GoFroggyRun, your pull request #684 looks good as far as it goes.
Can you also change the table title (which appears just above the table) along the lines suggested by this 673 comment?

@GoFroggyRun
Copy link
Contributor Author

@martinholmer thanks for catching this. I changed the title to Percentage Change in GDP:

screen shot 2017-10-05 at 12 53 37 pm

@martinholmer
Copy link
Contributor

@GoFroggyRun said:

I changed the title to Percentage Change in GDP.

Well, I guess the terms for different parts of a table can be confusing.

I would say that in commit 06f3747 you changed the row label, not the title.

What I would call the title is right above the year table row.

screen shot 2017-10-05 at 2 14 26 pm

What I was asking you to do was to change Elasticity of GDP wrt 1 - Average Marginal Tax Rate to something like Percentage Change in GDP.

The current title is simply incorrect. The table does not contain elasticities (the one assumed elasticity was specified on the Macro-Elasticity input parameter page) but rather contains the proportional change in GDP caused by the interaction of the assumed elasticity and the change in the average marginal tax rate.

Sorry about the confusion in my earlier request.

But we do need to change the incorrect title.

@MattHJensen @Amy-Xu @hdoupe

@martinholmer
Copy link
Contributor

Commit b27c6fa looks great! Thanks, @GoFroggyRun, and sorry for my confusing suggestions.

@GoFroggyRun
Copy link
Contributor Author

GoFroggyRun commented Oct 5, 2017

@martinholmer my bad, I thought your intention were to un-symbolize the % sign. I reverted the row label change and updated the title:

screen shot 2017-10-05 at 3 14 04 pm

@hdoupe
Copy link
Collaborator

hdoupe commented Oct 26, 2017

@GoFroggyRun LGTM. Is this ready to merge?

@GoFroggyRun
Copy link
Contributor Author

Yep @hdoupe

@hdoupe hdoupe merged commit 6e8dc46 into ospc-org:master Oct 26, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants