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

Working selector and defaults for InputLiteral using common reducers #426

Merged
merged 4 commits into from Mar 21, 2019

Conversation

jermnelson
Copy link
Contributor

@jermnelson jermnelson commented Mar 20, 2019

On the way to finishing ticket #412, this PR uses Redux to set defaults based on Resource Template ID and URI of property using more generic reducers and a single getProperty selector to map the redux state to the InputLiteral's props.

Fixes #390
Fixes #391
Fixes #428

@coveralls
Copy link

coveralls commented Mar 20, 2019

Coverage Status

Coverage decreased (-0.7%) to 76.537% when pulling 0d660f3 on inputliteral-selector into b36d952 on master.

Copy link
Contributor

@ndushay ndushay left a comment

Choose a reason for hiding this comment

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

again, I have eyeballed, and don't feel qualified to approve or not.

I notice we seem to have tests with vague descriptions that have multiple expect statements. I think it's a better practice mostly to avoid tests with multiple expects, unless they are multiple examples showing the same result ("returns true for valid JSON"), or multiple aspects of the same thing ("has exactly the expected RDF contents"). In the cases I call out below, the description of the tests is vague, and the specific expect statements do lend themselves to better descriptions of specific behavior.

Clearly not something we're going to comb over our existing tests to fix ... but it can be a really helpful practice when someone needs to edit the code 6 months from now and is wondering what the tests are supposed to be proving.

__tests__/components/editor/InputLiteral.test.js Outdated Show resolved Hide resolved
__tests__/reducers/literal.test.js Show resolved Hide resolved
__tests__/reducers/literal.test.js Show resolved Hide resolved
__tests__/reducers/literal.test.js Show resolved Hide resolved
__tests__/reducers/literal.test.js Show resolved Hide resolved
__tests__/reducers/literal.test.js Show resolved Hide resolved
src/components/App.jsx Outdated Show resolved Hide resolved
src/components/editor/InputLiteral.jsx Outdated Show resolved Hide resolved
src/components/editor/InputLiteral.jsx Outdated Show resolved Hide resolved
Copy link
Contributor

@jgreben jgreben left a comment

Choose a reason for hiding this comment

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

LGTM

src/reducers/index.js Show resolved Hide resolved
Fixes #390
Fixes #391

Working defaults for literal, follow through with updating state with hasItems using selectors

Working selector for InputLiteral on the way to using common reducers.

Fixes #390
Fixes #391
@ndushay
Copy link
Contributor

ndushay commented Mar 21, 2019

thanks for changing those test descriptions - I much prefer the new ones; hope it wasn't too time consuming. My question about that one test still stands -- is it testing what it says it's testing?

I'll leave it up to osh to merge; I'm not going to the mat over my remaining question.

@jgreben jgreben merged commit ef979d5 into master Mar 21, 2019
@jgreben jgreben deleted the inputliteral-selector branch March 21, 2019 22:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Development

Successfully merging this pull request may close these issues.

None yet

4 participants