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

Feature/disable and enabled props #640

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

whitphx
Copy link
Collaborator

@whitphx whitphx commented Jun 20, 2018

  • Update <DynamicForm /> and referAndGetBool (renamed from checkVisibility) to be able to handle disabled and enabled props which controls disabled state of each form field component.

@whitphx whitphx requested review from yuguanx and zreactor June 20, 2018 16:00
@whitphx whitphx force-pushed the feature/disable-and-enabled-props branch from 47c7149 to 0d6d6d1 Compare June 24, 2018 10:53
@whitphx whitphx force-pushed the feature/disable-and-enabled-props branch from 0d6d6d1 to cf67f6a Compare June 24, 2018 11:02
@@ -35,7 +35,7 @@ import {
} from '../../actions';

import {
checkVisibility,
referAndGetBool,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Does 'refer' in this method name refer to doctor referral? Slightly unclear what this method does...

return null;
}

// Translate "disabled" and "enabled" prop to boolean value
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe it is more appropriate to name something like checkEnabledToBool? (as it seems its main purpose is for check disable/enable -> bool output)

expect(checkVisibility(state, 'root', 'foo:aaa')).toBe(true);
expect(checkVisibility(state, 'root', 'foo:bbb')).toBe(false);
expect(referAndGetBool(state, 'root', 'foo:aaa')).toBe(true);
expect(referAndGetBool(state, 'root', 'foo:bbb')).toBe(false);
});

it('handles an empty array as falthy', () => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

falthy -> falsy

Copy link
Collaborator

@zreactor zreactor left a comment

Choose a reason for hiding this comment

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

Good and thorough test case.
However, property name orgProp & method name referAndGetBool are hard to understand the meaning of in the business logic.
Please consider clarification or different namings.

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.

2 participants