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

Added Choose/When/Otherwise components inspired by XSL #5

Closed
wants to merge 1 commit into from

Conversation

manuelbieh
Copy link

Added super useful Choose/When/Otherwise components as discussed on Twitter

@manuelbieh
Copy link
Author

Now that the package is growing we should consider adding Prettier to keep the style consistent!

Copy link
Owner

@leebyron leebyron left a comment

Choose a reason for hiding this comment

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

This seems pretty useful! I know XSL and JSTL use the Choose/When/Otherwise names, but what do you think about the names of similar control flow patterns? Switch/Case or Match/Case? Or even recycling the If/Else to be something like OneOf/If/Else? Too weird?

@@ -45,3 +45,16 @@ export function ElseIf(props: {
}): React.ReactNode;

export function Else(props: { children: React.ReactNode }): React.ReactNode;

export function Choose(props: {
children: When | Otherwise;
Copy link
Owner

Choose a reason for hiding this comment

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

I'm not a typescript wizard yet so I don't know what this should be, but I think you want to allow an ReactElement or Array of ReactElement of When | Otherwise type

return true;
} else {
return false;
}
Copy link
Owner

Choose a reason for hiding this comment

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

Couldn't this block just be:

find(child => child.type === When && child.props.test || child.type === Otherwise)

it('renders only the first when of which test is truthy', () => {
expectRenderToEqual(
<Choose>
<When test={true !== false}>
Copy link
Owner

Choose a reason for hiding this comment

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

just test={true}

@@ -28,3 +28,6 @@ declare function For<O: Object>({|
declare function If({ case: any, children: React$Node }): React$Node;
declare function ElseIf({ case: any, children: React$Node }): React$Node;
declare function Else({ children: React$Node }): React$Node;
declare function Choose({ children: React$Node<typeof When> | React$Node<typeof Otherwise>}): React$Node;
Copy link
Owner

Choose a reason for hiding this comment

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

Does this line work in a try flow?

I'd have expected React$Node<When | Otherwise>

<When test={someCondition}>
This will only be shown if someCondition is truthy.
</When>
<When case={otherCondition}>
Copy link
Owner

Choose a reason for hiding this comment

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

test not case?

@tomByrer
Copy link

tomByrer commented Apr 2, 2019

From an API standpoint, Choose/When/Otherwise is far better than nested ElseIf (which IMHO should be removed; makes my eyes hurt). But why not Switch/Case/Default which is familiar to far more JS devs?

@leebyron
Copy link
Owner

leebyron commented Apr 2, 2019

Following #12, I think I'm going to close this one, but I still like this feature from XSL! Maybe we should have react-conditionals library which includes these! I encourage you to create it ;)

@leebyron leebyron closed this Apr 2, 2019
@andrewfluck
Copy link
Contributor

I already started using the If component and now I gotta remove it :(

@andrewfluck
Copy link
Contributor

I would be down for a react conditionals lib

@andrewfluck
Copy link
Contributor

I'm in the process of claiming react-condition, Gonna put the If/Else/ElseIf code there. I'm gonna add in other conditional statements soon as well!

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.

None yet

4 participants