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

Feature request: render this.props.children instead of null when this.props.loading is false #26

Closed
weitjong opened this issue Feb 8, 2018 · 10 comments

Comments

@weitjong
Copy link

weitjong commented Feb 8, 2018

This is a feature request.

@davidhu2000
Copy link
Owner

@weitjong can you provide some use cases? I can't think of any use cases for this.

@weitjong
Copy link
Author

It has been some time already. I think the use case is something like this. I want to write:

render() {
    return (
        <ClipLoader loading={this.state.loading}>
            <other-child-component/>
        </ClipLoader>
    );
}

instead of

render() {
    if (this.state.loading) {
        return <ClipLoader loading={this.state.loading}/>;
    } else {
        return <other-component/>;
    };
}

Basically when the "loading" prop is false then the spinner component simply "goes away" and just renders the component's children in return (without needing to know what they actually are).

@GuillaumeCisco
Copy link
Contributor

@weitjong

You should write:

render () {
    const {loading} = this.state;

    return loading ? <ClipLoader /> : <OtherComponent>;
}

@weitjong
Copy link
Author

weitjong commented Jul 6, 2018

Yes, I could if the other child component is a single lone component. The above code is just an over simplification of my actual use case. The requested feature when accepted would allow me to nest children hierarchy just with plain XML. I could design the rendering for a whole page and when it takes too long to render, just wrap/nest the whole thing inside one of your component. A simple edit to do than have to refactor the code to inject the JS “if” or the ternary operator at the right place.

@GuillaumeCisco
Copy link
Contributor

@weitjong I think your case is not correct for wrapping.
Either you want a loading component, either a rendered content component. Using a parent wrapper as a loading component is a misunderstanding. Nothing has to live inside a loading component. It is not an HOC.

You can simply write things like that:

render () {
    const {loading} = this.state;

    return <div>
               {loading && <ClipLoader />}
               {!loading && <OtherComponent>}
          </div>;
}

Or If you really want an HOC for loading, you can create your own:

const myLoadingWrapper = (Loader) => {

        class C extends React.Component {
            render() {
                const {loading, children} = this.props;
                return loading ? <Loader /> : children;
            }
        }

        return C;
}
export default myLoadingWrapper;
import LoadingWrapper from './myLoadingWrapper';
import {ClipLoader} from 'react-spinners';

const ClipLoaderWrapper = LoadingWrapper(ClipLoader);

class Comp extends React.Component {
       render () {
            const {loading} = this.state;
            return <ClipLoaderWrapper loading={loading}>
                       <Other-child-component/>
                   </ClipLoaderWrapper>
      }
}

@weitjong
Copy link
Author

weitjong commented Jul 6, 2018

From the point of usability, I don’t think why not. Internally your component code already has an “if” check to determine whether to render a spinner or null. The request is simply to change the null to prop.children. It is backward compatible change. People that doesn’t like this could just use your component as it is now.

@GuillaumeCisco
Copy link
Contributor

@weitjong In fact, you are suggesting to introduce a future backward incompatible change. And a very bad practice.

@weitjong
Copy link
Author

weitjong commented Jul 6, 2018

Can you explain how it can introduce the so-called future backward incompatible change?

@GuillaumeCisco
Copy link
Contributor

@weitjong Using a wrapping interface, which is a bad practice in this case, will create a opinionated philosophy on this library which will include a future backward incompatible change.

@davidhu2000
Copy link
Owner

I have to agree with @GuillaumeCisco on this. Closing this for now.

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

No branches or pull requests

3 participants