[HOLD for payment 2020-01-03] [$2000] Allow Form component to detect children that are Class components

[puneetlath]

Problem

Our Form component is built to handle nested components and custom components within the form. That functionality is handled here. It seems, however, if the nested component is a Class component that the children of the class component aren't detected by the Form component. If the nested component is a Functional component, then it seems to work just fine. You can see this example here where I added Form to the WorkspaceNewRoom page and added two similar test components, one functional and one class based. The functional component is found by the form but the class based one isn't. Context Slack discussion: here.

Solution

Update this Form functionality so that it is able to find and recognize custom class-based sub-components.

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~019572d1017d216d70
• Upwork Job ID: 1602375844148887552
• Last Price Increase: 2023-01-03

[melvin-bot]

Current assignee @puneetlath is eligible for the Bug assigner, not assigning anyone new.

[melvin-bot]

Current assignee @puneetlath is eligible for the External assigner, not assigning anyone new.

[melvin-bot]

Job added to Upwork: https://www.upwork.com/jobs/~019572d1017d216d70

[melvin-bot]

Triggered auto assignment to Contributor-plus team member for initial proposal review - @sobitneupane (External)

[melvin-bot]

Current assignee @puneetlath is eligible for the External assigner, not assigning anyone new.

[fedirjh]

I think when component is class we should render it before we clone elements

diff --git a/src/components/Form.js b/src/components/Form.js
index 72ee4afc6b..7a61bb81df 100644
--- a/src/components/Form.js
+++ b/src/components/Form.js
@@ -193,6 +193,11 @@ class Form extends React.Component {
                 const nestedChildren = new child.type(child.props);
 
                 if (!React.isValidElement(nestedChildren) || !lodashGet(nestedChildren, 'props.children')) {
+                    if (nestedChildren.render) {
+                        return React.cloneElement(child, {
+                            children: this.childrenWrapperWithProps(lodashGet(nestedChildren.render(), 'props.children')),
+                        });
+                    }
                     return child;
                 }
 

[puneetlath]

@fedirjh just capturing what we've discussed in Slack so far: when I try with the WorkspaceNewRoom page that doesn't quite work. But doing children: this.childrenWrapperWithProps(nestedChildren.render()) does.

However, we have a new problem when we do that where the validation function isn't called when the Class input is changed/blurred (it is called for the functional component).

[mollfpr]

@puneetlath I just test @fedirjh proposal's. It seems working just fine for the class component but I think the changes that they made are too much to trace. I am more inclined if can just make small changes to handle the missing props.children and use the render function for the class component.

I'll leave it to you to decide.

[fedirjh]

@mollfpr basically I made change to this code block , the other part I just split the code for rendering form input to inputWrapperWithProps , so I can reuse it .

this is the code block I made changes

if (_.isFunction(child.type)) {
    const nestedChildren = new child.type(child.props);

    if (!React.isValidElement(nestedChildren) || !lodashGet(nestedChildren, 'props.children')) {
        return child;
    }

    return React.cloneElement(nestedChildren, {
        children: this.childrenWrapperWithProps(lodashGet(nestedChildren, 'props.children')),
    });
}

these are the changes

if (_.isFunction(child.type)) {
    let nestedChildren = new child.type(child.props);

    // If the custom component has a render method, use it to get the nested children
    if (_.isFunction(nestedChildren.render)) {
        nestedChildren = nestedChildren.render();
    }

    // If the custom component has children,
    if (lodashGet(nestedChildren, 'props.children')) {
        return React.cloneElement(nestedChildren, {
            children: this.childrenWrapperWithProps(lodashGet(nestedChildren, 'props.children')),
        });
    }

    // If the custom component has a single child, use it
    if (React.isValidElement(nestedChildren)) {
        return this.inputWrapperWithProps(nestedChildren);
    }

    // If the custom component has no children, return the component as is
    return child;
}

[sandipanghos]

puneetlath
@sobitneupane
sobitneupane

Is it still open for posting proposal?

[hungvu193]

@sandipanghos Yes, I believe that, you can submit proposal until one proposal is selected by the reviewers

[puneetlath]

Yes, as long as the help wanted label is there, you are free to submit proposals.

[sandipanghos]

Yes, as long as the help wanted label is there, you are free to submit proposals.

THANKS

[sandipanghos]

@sandipanghos Yes, I believe that, you can submit proposal until one proposal is selected by the reviewers

THANKS

[sobitneupane]

Looks like @mollfpr has worked in the related issue before and has already reviewed a proposal. @mollfpr You can take over C+ role if you want.

[fedirjh]

Updated Proposal

cc @puneetlath , @mollfpr

@puneetlath After further investigation , i found that custom inputs are not detected due to this line :
if (!React.isValidElement(nestedChildren) || !lodashGet(nestedChildren, 'props.children')) {
if nestedChildren === true and lodashGet(nestedChildren, 'props.children' === false , this means that we have a valid react element that should be evaluated instead of returning the child as it is , we have also the check if we have a class component , if so we have to render it to get it's children or to check if it's a valid react element

Solution 1

diff --git a/src/components/Form.js b/src/components/Form.js
index 72ee4afc6b..fe4a7506a4 100644
--- a/src/components/Form.js
+++ b/src/components/Form.js
@@ -190,12 +190,17 @@ class Form extends React.Component {
 
             // Look for any inputs nested in a custom component, e.g AddressForm or IdentityForm
             if (_.isFunction(child.type)) {
-                const nestedChildren = new child.type(child.props);
+                const childNode = new child.type(child.props);
+                const nestedChildren = _.isFunction(childNode.render) ? childNode.render() : childNode;
 
-                if (!React.isValidElement(nestedChildren) || !lodashGet(nestedChildren, 'props.children')) {
+                if (!React.isValidElement(nestedChildren) && !lodashGet(nestedChildren, 'props.children')) {
                     return child;
                 }
 
+                if (React.isValidElement(nestedChildren)) {
+                    return this.childrenWrapperWithProps(nestedChildren);
+                }
+
                 return React.cloneElement(nestedChildren, {
                     children: this.childrenWrapperWithProps(lodashGet(nestedChildren, 'props.children')),
                 });

Solution 2

    // Look for any inputs nested in a custom component, e.g AddressForm or IdentityForm
    if (_.isFunction(child.type)) {
        const childNode = new child.type(child.props);
        const nestedChildren = _.isFunction(childNode.render) ? childNode.render() : childNode;

        if (React.isValidElement(nestedChildren) || lodashGet(nestedChildren, 'props.children')) {
            return this.childrenWrapperWithProps(nestedChildren);
        }

        return child;
    }

Solution 3

    // Look for any inputs nested in a custom component, e.g AddressForm or IdentityForm
    if (_.isFunction(child.type)) {
        const nestedChildren = ((nested = new child.type(child.props)) => (_.isFunction(nested.render) 
            ? nested.render() 
            : nested))();

        if (React.isValidElement(nestedChildren) || lodashGet(nestedChildren, 'props.children')) {
            return this.childrenWrapperWithProps(nestedChildren);
        }

        return child;
    }

[mollfpr]

Thanks, @sobitneupane! Yeah, it’s kinda awkward but I’ll let @puneetlath decide 👍

[mollfpr]

@fedirjh proposal Solution 2 looks good to me.

Maybe add a comment about the render() is used for.

cc @puneetlath

[melvin-bot]

📣 @mollfpr You have been assigned to this job by @puneetlath!
Please apply to this job in Upwork and leave a comment on the Github issue letting us know when we can expect a PR to be ready for review 🧑‍💻
Keep in mind: Code of Conduct | Contributing 📖

[puneetlath]

@sobitneupane yes I think it makes sense for @mollfpr to be the C+ on this one. Thanks!

@mollfpr @fedirjh I agree proposal 2 (with an explaining comment). It's nice and clean.

@fedirjh how would you like to implement that solution plus refactoring the WorkspaceNewRoom component to use the Form component in one PR. And we can update the bounty to $2k.

Then after you've done that, I'll update the form validation as described here. But for your PR, we shouldn't change any of the form functionality. Just update it to use the Form component, which it will now be able to do because your proposal will allow it to detect Class component children.

[mollfpr]

@fedirjh how would you like to implement that solution plus refactoring the WorkspaceNewRoom component to use the Form component in one PR. And we can update the bounty to $2k.

I think it's a great idea. We also can test this issue with the refactor.

[fedirjh]

@fedirjh how would you like to implement that solution plus refactoring the WorkspaceNewRoom component to use the Form component in one PR. And we can update the bounty to $2k.

@puneetlath That's fair . I am ready to implement that + Refactoring the WorkspaceNewRoom in one PR . I will comment here once PR is ready.

[melvin-bot]

📣 @fedirjh You have been assigned to this job by @puneetlath!
Please apply to this job in Upwork and leave a comment on the Github issue letting us know when we can expect a PR to be ready for review 🧑‍💻
Keep in mind: Code of Conduct | Contributing 📖

[puneetlath]

Awesome!

Upwork job is here as well @mollfpr @fedirjh.

[fedirjh]

@puneetlath while refactoring WorkspaceNewRoomPage , I found that RoomNameInput is used in ReportSettingsPage , I think that we have to refactor that component too .

App/src/pages/ReportSettingsPage.js

Lines 194 to 201 in aa16054

	<RoomNameInput
	ref={el => this.roomNameInputRef = el}
	value={this.state.newRoomName}
	policyID={linkedWorkspace && linkedWorkspace.id}
	errorText={this.state.errors.newRoomName}
	onChangeText={newRoomName => this.clearErrorAndSetValue('newRoomName', newRoomName)}
	disabled={shouldDisableRename}
	/>

[fedirjh]

WorkspaceNewRoomPage Refactor is done , PR will be ready to review once other screenshots are added

WorkspaceNewRoomPage.mov

[fedirjh]

@puneetlath , @mollfpr PR is ready #13713

[melvin-bot]

@puneetlath, @mollfpr, @fedirjh Whoops! This issue is 2 days overdue. Let's get this updated quick!

[mollfpr]

The PR is in production. Starting the regression period!

[puneetlath]

@mollfpr can you apply here? https://www.upwork.com/jobs/~019572d1017d216d70

[mollfpr]

@puneetlath I think the bounty is updated to $2k ref here and the PR is eligible for the bonus.

[melvin-bot]

Upwork job price has been updated to $2000

[mollfpr]

@puneetlath applied!

[puneetlath]

@fedirjh has been paid.

Offer has been sent for @mollfpr.

[mollfpr]

@puneetlath accepted 👍

[puneetlath]

All paid with 50% bonus. Thanks y'all!