-
Notifications
You must be signed in to change notification settings - Fork 169
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
React 15.4 compatibility; don't rely on internal modules #105
Conversation
@STRML actually it's not that easy. For example, I think your solution won't work with plain number values that React coerces to strings with So this PR would be a breaking change. |
That's true. We could write that in or rely on a module. There are likely other edge cases as well; how far do we want to go to cover them all? |
In my opinion, a better solution would be a new major release without support to setting That container is created outside React, so I guess it's more rational to not rely on it at all. |
32e8945
to
fa613de
Compare
That sounds good to me. I've updated the PR to remove className/style support entirely. |
Brings React 15.4 compatibility; doesn't rely on any internal React modules. If you need to style the portal, wrap your children in another div.
fa613de
to
e020330
Compare
Whoops, forgot to update the tests. CI should be green now. |
Thanks @STRML! Just a note about the drop of Probably we would use // [...]
return (
<Portal closeOnEsc closeOnOutsideClick>
<div>
<h2>Title depending on {this.props.someProp}.</h2>
<p>Another thing depending on {this.state.someState}.</p>
</div>
</Portal>
); So the prop Except for edge cases like this: const NEVER_CHANGE_CHILDREN = (
<div>
<p>Really static content.</p>
</div>
);
// [...]
return (
<Portal closeOnEsc closeOnOutsideClick>
{NEVER_CHANGE_CHILDREN}
</Portal>
); So this drop is really like nothing in terms of rendering. |
It would be less useless in combination with babel-plugin-transform-react-constant-elements; however, since the |
Thanks! I agree with removing |
Thanks @tajo! 😃 |
Hey. I don't really undestand why style/className support has been removed. Personnally I rely on this, and I find it more convenient to have a className directly on the portal node than on a child node. How am I supposed to position the portal correctly if I can't craft a selector on it? Are you all using position fixed on a portal's child? Because I absolutly can't do that on my own app, so I need correct positionning of the portal. Removing usage of internal modules does not seem related to me to supporting className and style attributes. Why breaking compatibility on this? Also, why remove this line ? |
I've made a PR to support className again |
Also I'd like to mention that stuff like shouldComponentUpdate shouldn't have to be removed too IMHO. (but I'm not sure it's that useful anyway because for portals it's likely the children will always change, and it's the children that should rather optimize rendering directly) If you want to remove React internal code usage it's fine, but it does not mean to introduce unnecessary breaking changes. I'm sure most lib users would be fine if you copy some lines of the react internal code you rely on to maintain compatibility (as a temporary solution, not saying lib size is not important). I'd rather have a little bigger lib than a non-compatible lib :) But in this case I'm not sure it's really worth it. shouldComponentUpdate is not that useful for portals, and I don't think many people use JS inline styles with portals. (However I think it would be nice to support CSS-IN-JS frameworks like styled/glamor and allow to pass className + data-attributes to portal node.) |
@slorber I guess this decision became from “just support React 15.4” to something like “improve the library design”. And that's the main reason for the new major release with breaking changes. And, of course, all correction needed is pretty obvious: <Portal className="my-class" style={{ color: '#F00' }}>
{content}
</Portal>
// becomes:
<Portal>
<div className="my-class" style={{ color: '#F00' }}>
{content}
</div>
</Portal> The portal wrapper is created directly with some It's not good that the wrapper looked like a normal element (that you can set |
@daltones I do not agree with that decision and I think it should rather be discussed with the community. Personnally I'm interested to have both React 15.4.1 support, and className support for my app, but with 3.0.0 release it was simply not possible. Also I think it's really annoying to have "divs" with no attributes while debugging your dom because you can't see immediately it's a portal and which one it is. When your app may have a lot of portals at the same time it's quite annoying. Also it's not consistent with what other portal libs are doing, as these solutions allow classnames (and sometime styles) on the portal node:
I would also say that if people have different opinions on the subject, it's worth covering everybody's opinion. If you think it's not elegant to put a className on a portal node, then don't, but please let me do it :) |
Okay, let's see what @tajo says about rolling back But I don't think we're trying to reach some consistency with those libraries at all. I mean, consistency with what? They will not work together, they're just different solutions. We tried to follow the principle of separate concerns here: the portal does its job rendering something somewhere else; that thing styles itself. That's it. |
I agree with @slorber . If you don't want the portal to have a class, do not add it. But let me decide if I want to add a class to mine. The example above would in my case look like:
I understand what you are getting at with the principle of concern @daltones. But in this case I do not want to have an "unnamed" div in the DOM because when we are building our framework we need to give classes to all of the framework elements. |
This also forces me to add a ref to the extra div for animation in for example beforeClose. It was easier before because you recieved the node passed in as an argument and could use it directly for animation. Just as shown in the examples. I still recieve the node but now I need to get the DOM node for the new div. |
I'm sorry this disrupted your workflow, but breaking changes is the entire point of a major release. We found that by disabling className/style support we could significantly shrink the size of this library by more than 50% including dependencies - there is no guarantee that Re: Portal certainly will not and should not natively support css-in-js frameworks. It's a matter of scope: do one thing, do it well. The whole point of composable components is centered around this concept.
Exactly. The root component was fundamentally broken as a pseudo-DOM element; it only supported className and style, and setting and resetting them required reusing internal React code. It simply wasn't needed. I'm sorry this caused you all some work, but the result is that Portal is a much more predictable and svelte library now. |
I am with @STRML on this. I think that adding |
Actually I'm not really discussing about anything else than className. I'm ok for removing shouldComponentUpdate and having a breaking change on inline styles if that's complicated, but I'd like to be able still use a className:
Also sometimes @tajo we work in teams where the integration team has done some CSS, and if I come to them and tell them "I'm sorry the JS lib I'm using forces me to create an extra useless wrapper div element with no attributes, you have to rewrite some selectors because of the lib I decided to use", it's not cool (and yes, we may use Personnally I don't think it's a good idea to consider that the node on which you mount the portal is to be considered as equivalent to the react mount node. There is only one react app, and it is known to have a single mount node. The portals are implementation details of that react app, so why should they be considered somehow as different extra react apps inside my app from an exterior point of view? As @STRML seems to be ok to support className what about his suggestion of warning when passing style/onX props that are unexpected, so that the user knows and isn't surprised about unsupported properties? Would this get merged? |
If it was used a lot there would be more people here talking about that (the lib has now 50k/month downloads...).
Not really a big deal. React devs create a lot of extra divs anyway.
👌
Could be a className attached to your extra/root div? Besides, this doesn't seem as a valid use-case anyway (maybe if you combine React with some other libs).
Can you explain? What's the use case? What's the "not out of the box" solution?
That's not an argument. At all.
No, sorry. Unless there are 10+ people up-voting that and having better arguments. Then, I would think about that again. |
@tajo I understand your points but just these arguments should be largely enough:
I agree that this feature is not absolutely required to build portals, and my request is mostly for developer experience and ease of migration, but by doing this change, you make the React 15.4 migration path a bit harder for those of us who use a lot of complex portals in our app. In my situation, I'm unable to upgrade React without refactoring all my portals CSS, taking the risk of breaking things. And yes, on a complex legacy app, it can be significantly more difficult than just adding a new extra div and changing 1 css selector. The release is very recent, so let's see what people think over time, in the meantime, here's a fork people can use if they want to migrate to React 15.4.0 without having to refactor their portals:
|
Also note that the Portal component only passes down the "closePortal" callback if the element is not a dom element, which means that <Portal className="clazz">
<PortalContent/>
</Portal> Can't be replaced out of the box by <Portal>
<div className="clazz">
<PortalContent/>
</div>
</Portal> Otherwise the PortalContent would never receive that callback. A general solution can be: <Portal>
<PortalWrapper className="clazz">
<PortalContent/>
</PortalWrapper>
</Portal> Where PortalWrapper is: const PortalWrapper = ({
children,
closePortal,
className,
...rest
}) => {
let childrenUpdated = children;
if (typeof childrenUpdated.type === 'function' && closePortal) {
childrenUpdated = React.cloneElement(childrenUpdated, {closePortal: closePortal});
}
return (
<div
className={classnames('portal', className)}
{...rest}
>
{childrenUpdated}
</div>
);
}; For 10 lines of code removed in the lib, you make the React upgrade more painful and risky. I'm not saying you should not remove the className support (I agree that unability to add a onClick listener is disturbing and I've tried to do it), but at least please make a 3.x release with it, and maybe remove it later in 4.x. People could at least still upgrade React without too much pain, and decide for themselves if they want to upgrade lib to 4.x if they don't absolutely need className support. |
Since this has been referenced from multiple issues, I want to add that even though react-portal does not allow setting a class name, it would be beneficial to have a class name for the component. Currently, there is no way to target the containing My use case is supporting a modal on iOS Safari. (CSS) Viewport units aren't functional/reliable with the expanding/collapsing menus and without being able to add |
@devinmcinnis See https://github.com/STRML/react-portal-minimal, where I've rewritten this component. It supports |
@STRML I'm not sure to understand the situation :D You were the one removing className support in this lib and said the following:
Yet you support className in your forked library? why ? |
I changed my view - I basically wanted to get the root element out of any business of passing along normal DOM attributes, but I found that See also #113 (comment). The point is to stop the support of DOM attributes at |
Hmm yeah didnd't remember your comment here ;) |
Setting
style
is easy enough to do with standard DOM apis,and the shouldComponentUpdate is unnecessary as render is
extremely quick.