Skip to content

Update Frame to allow use of React.Fragment - #57

Merged
nkzawa merged 5 commits into
vercel:masterfrom
jsjoeio:master
May 10, 2019
Merged

nkzawa merged 5 commits into
vercel:masterfrom
jsjoeio:master

Conversation

@jsjoeio

@jsjoeio jsjoeio commented Mar 17, 2019 •

Copy link
Copy Markdown
Contributor

Intro

Hi there!

First, thank you for creating this 🙏 I was looking for an easy way to animate using React components.

Problem

I wanted to be able to use <React.Fragment /> as the rendered component so that the components inside of Frame could be rendered without a wrapper essentially.

My hope is to use this to animate parts of an SVG component in React.

Solution

Inside frame.js check if component === 'Fragment', if so, render inside <React.Fragment />

I tested this in a Codesandbox and it seems to be working: https://codesandbox.io/s/5309430n5x

Notes

This is my first OSS PR that is not related to docs so please be kind in any feedback you have 😄 Happy to make changes based on any feedback! Not sure if this is the best way to approach the problem but thought I'd submit the PR and listen to any feedback you have.

PS...I didn't write any tests but we probably should. I wasn't sure exactly what to test. If you have advice there, also open to it.

Comment thread readme.md Outdated

- Use `import { Keyframes } from 'react-keyframes'` or `require('react-keyframes').Keyframes`.
- The `component` prop specifies what component `Keyframes` renders as.
- The `component` prop specifies what component `Keyframes` renders as. Passing "Fragment" will render the component inside a `<React.Fragment />`. If no component is passed, the default is a "span" tag.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

It seems we can pass a React component to React.createElement so that <Keyframes component={React.Fragment}> just work. Maybe we should make it clear on README.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I wonder if we should support a "Fragment" string too. What do you think?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Ah! I didn't realize that. I updated the docs.

Also, I don't think we need to? I removed it from my code because doing <Keyframes component={React.Fragment}> seems to be sufficient. What do you think?

@nkzawa
nkzawa merged commit bc9625b into vercel:master May 10, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

2 participants