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

Tweaks to new Wrapperless mode #204

Merged
merged 6 commits into from
Nov 26, 2017
Merged

Tweaks to new Wrapperless mode #204

merged 6 commits into from
Nov 26, 2017

Conversation

joshwcomeau
Copy link
Owner

Some small tweaks to @tobilen's excellent work creating a
wrapperless mode for React Flip Move with React 16.

  • Removed the anchor, as this will be computed based on the parent
  • Removed the warning for not supplying an anchor
  • Update README
  • Add details to API reference
  • Fix bug with the first transition being buggy, by setting the parentData on mount
  • Extract findDOMContainer method
  • Switch from false to null for typeName
  • Update flow-typed defs
  • Update package.json version number

Some small tweaks to @tobilen's excellent work creating a
wrapperless mode for React Flip Move with React 16.

- Removed the `anchor`, as this will be computed based on the parent
- Removed the warning for not supplying an anchor
- Update docs
- Fix bug with the first transition being buggy, by setting the parentData on mount
- Extract `findDOMContainer` method
* [Contributions](#contributions)
* [Development](#development)
* [Flow support](#flow-support)
* [License](#license)
Copy link
Owner Author

Choose a reason for hiding this comment

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

This TOC was quite outdated, and I realized it'll always be problematic. This doc isn't so long now that API Reference is its own thing, so I ditched the TOC.

Wrapperless mode is nice, because it makes FlipMove more "invisible", and makes it easier to integrate with parent-child CSS properties like flexbox. However, there are some things to note:

- This is a new feature in FlipMove, and isn't as battle-tested as the traditional method. Please test thoroughly before using in production, and report any bugs!
- FlipMove does some positioning magic for enter/exit animations - specifically, it temporarily applies `position: absolute` to its children. For this to work correctly, you'll need to make sure that `<FlipMove>` is within a container that has a non-static position (eg. `position: relative`), and no padding:
Copy link
Owner Author

Choose a reason for hiding this comment

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

This is something I noticed in Storybook; It's kind of a bummer :( and makes me think that wrapperless mode will likely cause more trouble than it's worth... but I figure we'll see. If we get a ton of confusing issues, maybe we can add a check that console.warns when the parent DOM element isn't configured correctly.

Copy link
Collaborator

@tobilen tobilen Nov 25, 2017

Choose a reason for hiding this comment

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

since we already have the dom node, those styles could be overwritten after findDOMNode:

  findDOMContainer = () => {
    // eslint-disable-next-line react/no-find-dom-node
    const domNode = ReactDOM.findDOMNode(this);
    const parentNode = domNode && domNode.parentNode;

    // This ought to be impossible, but handling it for Flow's sake.
    if (!parentNode || !(parentNode instanceof HTMLElement)) {
      return;
    }

    // Make sure parentNode has position relative and no padding
    parentNode.style.position = 'relative';
    parentNode.style.padding = 0;

    this.parentData.domNode = parentNode;
  };

should be mentioned in the docs though.

Copy link
Collaborator

Choose a reason for hiding this comment

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

parentNode.style.position = 'relative';

We can do that only if window.getComputedStyle(parentNode) === 'static')
And we can't change the padding as it would break the layout

Copy link
Owner Author

Choose a reason for hiding this comment

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

Yeah, so I thought about applying a "default" relative position if it isn't set to something other than static, but it feels a bit like it crosses a line; we'd be mutating nodes outside of Flip Move's control. It could cause confusion for users if they have absolutely-positioned things down-tree.

So either we apply position: relative, which fixes FlipMove but breaks their layout, or we do nothing, and it breaks enter/exit animations. The latter is clearly a FlipMove issue, and so they'll come here and we can advise them... but the former might appear to them as a CSS issue / they won't understand why things are broken, why a random position: relative is being applied to their div...

Maybe we can apply relative positioning if it's static, and also console.warn to let them know what's happening? But, if the user doesn't need enter/leave animations, they don't need this position: relative, and so the warning would just be console spam.

That said, it's really rare that position: static is actually desirable (I've seen a trend where people just add a * { position: relative } global CSS rule), so I think it's probably best to optimize for the common usecase.

Gonna do it in a subsequent PR to make reviews easier, but I'll apply @Hypnosphi's static check, and issue a console.warn when the change is necessary.

| `String` | 'div' |
| **Accepted Types:** | **Default Value** |
|----------------------|-------------------|
| `String` | `null` | 'div' |
Copy link
Owner Author

Choose a reason for hiding this comment

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

I mentioned this in #202, and didn't see any strong opinions; my preference is that we use null over false, since it feels more "pure" semantically.

If y'all think there's value in also supporting false (or if there's an argument I haven't thought of for why false makes more sense?), happy to update :)

Copy link
Collaborator

Choose a reason for hiding this comment

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

I prefer null

@@ -1,23 +1,23 @@
declare module "react-flip-move" {
declare module 'react-flip-move' {
Copy link
Owner Author

Choose a reason for hiding this comment

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

Argh, Prettier updated all the formatting here, adding trailing commas and single-quotes.

@Hypnosphi does this matter?

Also, I renamed this file from 2.9 to 2.10, as the version will change. Should there be a flow-typed file for every version?

Copy link
Collaborator

Choose a reason for hiding this comment

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

yeah, pretty sure there should be. we don't want to allow null as a value on versions lower than 2.10.x

you can use wildcards in the name to span multiple versions though, like we're currently doing with the patch version.

Copy link
Collaborator

@Hypnosphi Hypnosphi Nov 25, 2017

Choose a reason for hiding this comment

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

does this matter?

If it doesn't for you, you shouldn't use prettier =) And I believe, trailing commas are one of the few things that are configurable

Copy link
Owner Author

Choose a reason for hiding this comment

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

yeah, pretty sure there should be. we don't want to allow null as a value on versions lower than 2.10.x

Yep, makes sense, thanks!

If it doesn't for you, you shouldn't use prettier =) And I believe, trailing commas are one of the few things that are configurable

You misunderstand; I was asking specifically about flow-typed. Wanted to make sure there wasn't a different community convention for these type files (or, worse, third-party tools that don't understand trailing commas)

But yeah, sounds like it's a non-issue, so I'll go ahead.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Well we kinda introduce new API, so it requires an updated libdef

Copy link
Collaborator

Choose a reason for hiding this comment

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

And I think we should disable eslint for flow-typed directory so that it’s easier to sync it with the version in flow-typed repo

@joshwcomeau joshwcomeau requested a review from tobilen November 25, 2017 14:13
@joshwcomeau joshwcomeau merged commit 784604d into master Nov 26, 2017
@joshwcomeau joshwcomeau deleted the publish-2.10.0 branch November 26, 2017 15:03
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.

3 participants