-
Notifications
You must be signed in to change notification settings - Fork 662
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
Support posing regular react components #276
Support posing regular react components #276
Conversation
@@ -4,8 +4,6 @@ description: Create a posed component | |||
category: react | |||
--- | |||
|
|||
**Note:** React Pose is built with React 16.3.0, which is currently in alpha. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
16.3.0 was released this morning 🎉
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was about to create a PR for this right now 😄
But I think it should be better to only remove the part of the sentence behind the comma instead of removing the whole line. Otherwise some users might be confused when trying to use it with React < 16.3.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@arcticicestudio This PR also adds the backwards compatibility polyfill so it should work with older versions of React
@@ -76,13 +79,18 @@ export class PoseElement extends React.PureComponent<PoseElementProps> { | |||
} | |||
} | |||
|
|||
getRef(): Element { | |||
return this.hostRef || this.innerRef || this.ref; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Order here is important: Favouring hostRef
(facebook/react#11401) first, then any innerRef
(for Styled Components support), then the original ref
.
I'm not sure if the values could get out of date at all? Is there ever a case where the innerRef gets updated, but the hostRef doesn't? It all feels a bit icky, but less icky than using .findDOMNode()
(which also doesn't work with Fragments).
}, | ||
children | ||
)} | ||
</PoseParentContext.Provider> | ||
); | ||
} | ||
} | ||
|
||
reactLifecyclesCompat(PoseElement); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Give backwards compatibility with existing react code bases.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey man, thanks for the PR!
I love the idea of posed(AnyComponent)
and I think this is a good first attempt but I don't think this specific approach will work. Having to account for and also explain the whole hostRef
/innerRef
stuff is causing noise in both the code and the docs and I think that's triggering my spider senses and is the reason I've so far avoided it.
My experience with Popmotion and people's complaints that its too complicated to use, with Pose I'd rather keep implementation simple and abilities limited, layering in complexity when it can be exposed simply.
Although it's preferred not to use findDOMNode
, from both a conceptual and consumption point of view it's much easier to use and explain. There's only the overhead of explaining "don't use this with fragments!" and the need to pass on style
props (for FLIP stuff)
Whereas by doing this via refs, introducing hostRef
to work around innerRef
/ref
and the reason they're not properly passed along, we're getting well into the weeds of how React works and its limitations. We're even writing around and explaining the limitations of Styled Components.
There's a new API in React 16.3 called forwardRef
and I wonder if it can help? Or perhaps better, if you use findDOMNode
, can we make this same posed(MyComponent)
API completely transparent to users the same way Styled Components is? That way, the same way SC says "please pass on className
", we can simply say "please pass on style
"?
Again I think this is an excellent idea but I've gotta make sure the API is kept simple, preferably invisible. If you need to bounce any ideas off me it'd be great to talk about this further because I think getting this right will be very powerful.
Yeah, I went back and forward over using
A combination of points 2 & 3 could be very confusing - which element should receive the At this point, the documentation / API would become: #### Calling `pose` with a React component
`pose` is called directly, passing in [posed props](/pose/api/props):
``javascript
class MyComponent extends React.Component {
/* ... */
}
const DraggableComponent = posed(MyComponent)({
draggable: 'x',
dragBounds: { left: 0, right: 100 }
})
export default ({ radius }) => <DraggableComponent r={radius} />
``
**Note:** React Pose requires a single DOM element to correctly calculate animations,
as such using `Fragment`s is not supported.
**Note 2:** Custom styles are applied to the DOM node being animated.
When calling `pose` with a React component, it is up to you to correctly apply `props.style` to the DOM node:
``javascript
class MyComponent extends React.Component {
render() {
<div style={this.props.style}>Drag me</div>
}
}
``
I feel this leaves us in the same position as before, but using the non-recommended Using I guess the question comes down to where you want the trade-offs: "you must always pass Edit: I hope the above didn't come across as negative / too defensive... I'm 100% ok with throwing out this solution and trying something else, I want to see the functionality exist in whatever form it may take on, and had lots of fun poking around, learning TypeScript and getting familiar with your awesome codebase :) |
99750a4
to
168bc7a
Compare
I've rebased onto |
I've published 168bc7a |
@jesstelford thanks for that I'll give it a go now! |
168bc7a
to
5780002
Compare
👍 I've rebased onto |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey man, I've made the following changes:
- Simplified the docs a tad
- Our only internal reference to
ref
isref
- Removed
setInnerRef
andsetHostRef
- we just pass onsetRef
aliased
@@ -100,11 +108,11 @@ export class PoseElement extends React.PureComponent<PoseElementProps> { | |||
// If we're popping this element out from the DOM flow, build | |||
// and apply position: absolute styles that visually match the previous | |||
// location in the DOM | |||
if (popFromFlow && this.ref && this.ref instanceof HTMLElement) { | |||
if (popFromFlow && this.getRef() && this.getRef() instanceof HTMLElement) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Stick to this.ref
- it doesn't matter where we got this from.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unfortunately, order matters with the ref. Check out this example:
class SomeComponent extends React.Component {
render() {
return (
<h1 ref={this.props.innerRef}>Hello World</h1>
)
}
}
class Posed extends React.Component {
setInnerRef = () => {
console.log('this.ref now points to <h1>');
}
setRef = () => {
console.log('this.ref now points to <SomeComponent>');
}
render() {
return (
<SomeComponent ref={this.setRef} innerRef={this.setInnerRef} />
)
}
}
ReactDOM.render(
<Posed />,
document.getElementById('root')
);
Gives output:
this.ref now points to <h1>
this.ref now points to <SomeComponent>
Which brings us back to where we started (and the reason for this PR).
We could alias this.innerRef
to this.hostRef
without issue, so I'll make that change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I’m not sure I fully understand the problem on this one? this.ref is an internal reference for us that we expose via hostRef. It’s up to the user to ensure that’s passed correctly the the host element rather than the component (if it’s using forwardRef for instance both of those refs will be h1)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh I see. It’s late 😁
Maybe we should only pass on hostRef for now as per docs, maybe innerRef if we’re srue it won’t be a problem
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Turns out trying to get to sleep is great at figuring this shit out (again). I see, actually this time. Proceed. 🙃
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wait wouldn’t it be possible to check if elementtype was a string and only send ref if it was, otherwise it’s a component and sent hostRef and innerRef?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, of course! I'll give that a shot 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Your tired self has good ideas too ;)
if (!this.popStyle) { | ||
props.style = { | ||
...props.style, | ||
...calcPopFromFlowStyle(this.ref) | ||
...calcPopFromFlowStyle(this.getRef() as HTMLElement) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As before
@@ -143,10 +159,10 @@ export class PoseElement extends React.PureComponent<PoseElementProps> { | |||
|
|||
// If first in tree | |||
if (!registerChild) { | |||
this.initPoser(poseFactory(this.ref, props)); | |||
this.initPoser(poseFactory(this.getRef(), props)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Revert
setRef = (ref: Element) => { | ||
const { innerRef } = this.props; | ||
if (innerRef) innerRef(ref); | ||
this.ref = ref; | ||
}; | ||
|
||
componentDidMount() { | ||
if (!this.ref) return; | ||
if (!this.getRef()) return; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Revert
} else { | ||
registerChild({ | ||
element: this.ref, | ||
element: this.getRef(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Revert
@@ -117,14 +125,22 @@ export class PoseElement extends React.PureComponent<PoseElementProps> { | |||
return props; | |||
} | |||
|
|||
setInnerRef = (ref: HTMLElement) => { | |||
this.innerRef = ref; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove both of these
@@ -98,10 +135,10 @@ const sidebarProps = { | |||
closed: { x: '-100%' } | |||
} | |||
|
|||
const Sidebar = styled(posed.nav(sidebarProps))` | |||
const Sidebar = posed(styled.nav` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Leave this as before - I'd like to encourage the previous syntax because it leaves the complexity on Styled Components.
@@ -26,6 +24,12 @@ import posed from 'react-pose' | |||
|
|||
### Create a posed component | |||
|
|||
`pose` can be used in two ways: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
### Create a posed component
`posed` can be used to create posed components in two ways:
1) **Recommended:** Create HTML & SVG elements (eg `posed.div()`)
2) **Advanced:** Convert existing components (eg `posed(Component)()`)
#### HTML & SVG elements
@@ -37,6 +41,39 @@ const DraggableCircle = posed.circle({ | |||
export default ({ radius }) => <DraggableCircle r={radius} /> | |||
``` | |||
|
|||
#### Calling `pose` with a React component | |||
|
|||
`pose` is called directly, passing in [posed props](/pose/api/props): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Existing components
Existing components can be converted to posed components by calling posed
directly:
const PosedComponent = posed(MyComponent)(poseProps)
For performance and layout calculations, React Pose requires a reference to the underlying DOM element. So, the component to be posed must pass hostRef
to the host element's ref
prop.
const MyComponent = ({ hostRef }) => <div ref={hostRef} />
const PosedComponent = posed(MyComponent)({
draggable: true
})
export default () => <PosedComponent pose={isOpen ? 'open' : 'closed'} />
For FLIP support in a PoseGroup
, it optionally needs to pass on the style
prop:
const MyComponent = ({ hostRef, style }) => <div ref={hostRef} style={style} />
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Excellently worded, thanks!
getSetProps() { | ||
const { | ||
children, | ||
elementType, | ||
poseProps, | ||
onChange, | ||
innerRef, | ||
hostRef, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove
I've pushed those tweaks, but will have to work through the merge conflicts before this branch is ready again. Thanks for the review! The only outstanding issue now is: #276 (comment) |
Nice one thanks for taking another look at his! |
Quick update: Was banging my head on the desk for many an hour trying to figure out why I was getting a Am attempting the rebase / checking if the type is a string now. |
6d60c37
to
2fbc0a1
Compare
Phew! What a journey. Ok, got the changes pushed. Have published it as EDIT Getting the codebase setup again / rebased onto #!/bin/sh
echo "CMD: lerna clean"
lerna clean
echo "CMD: cd packages/popmotion-pose && npm install --save-dev style-value-types stylefire"
cd packages/popmotion-pose
npm install --save-dev style-value-types stylefire
cd -
echo "CMD: cd packages/react-pose && npm install --save popmotion"
cd packages/react-pose
npm install --save popmotion
cd -
echo "CMD: lerna bootstrap"
lerna bootstrap
echo "CMD: cd packages/popmotion && npm run build"
cd packages/popmotion
npm run build
cd -
echo "CMD: cd packages/popmotion-spinnable && npm run compile"
cd packages/popmotion-spinnable
npm run compile
cd -
echo "CMD: cd packages/popmotion-react && npm run compile"
cd packages/popmotion-react
npm run compile
cd -
echo "CMD: cd packages/react-pose && npm run build && npm run bundle"
cd packages/react-pose
npm run build && npm run bundle
cd - |
You’re an actual legend. I installed Leona but never took advantage. And broke all our code pens with that React bug, that was a sweaty moment 😅 I’ll merge and release at some point today, thanks for your hard work on this |
Haha, thanks mate :) Be sure to test it out with any example apps, etc, you've got - I don't want to have crept any bugs in there (especially since I didn't write any tests). |
🎉 |
@InventingWithMonster just gave Here's a quick example showing the errors: https://github.com/jesstelford/react-pose-issue It also throws warnings when posing a functional component (since it tries to attach the Edit Interestingly the error doesn't happen with create-react-app - I'm not sure what the difference is between the hot module setup, but I was only able to reproduce the issue with a next.js project. |
Hey man thanks for the working demo, il take a look at work. I was playing
around seeing how little code I could get away with for this, and that
seemed to work pretty well. Also the innerRef stuff I decided was always
going to be used to fish out the underlying DOM node (as per my head docs
as I forgot to add them to the actual docs) which is why that logic went
What’s happening here is (as I’m sure you encountered) one of the refs is
coming through as a React component. I’ll look into why. I also don’t
understand the functional component error as now the only time we sent ref
is if elementType is a string. I’ll put back the explicit functional
component check
…On Fri, 13 Apr 2018 at 03:56, Jess Telford ***@***.***> wrote:
@InventingWithMonster <https://github.com/InventingWithMonster> just gave
***@***.*** a go, but am getting errors with the code setup I was
using. Some investigation shows that a bunch of the code got clobbered in
6965721
<6965721>
- not sure if this was intentional?
Here's a quick example showing the errors:
https://github.com/jesstelford/react-pose-issue
It also throws warnings when posing a functional component (since it tries
to attach the ref prop).
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#276 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AHfLKq5Wzv8w-1xBz5WR3WVyzHrrnxGCks5toBPpgaJpZM4TBUPe>
.
|
Thanks for investigating 👍 |
@jesstelford Hey man Fixed the error with 9a1027e#diff-57a535dfcce9662e7d53f9cdff1b0073R163 As a heads up the functional ref warning is still there, which I suspected as I don't think there's a way of detecting a Styled Component functional component and was still getting with |
Just wanted to let ya know it looks like the |
The same way Styled Components does it:
I had some trouble getting the monorepo setup and running, but got there in the end - perhaps a setup guide might help future contributors?
Also, this is my second time ever writing TypeScript - I hope I didn't butcher it too bad!