Skip to content

Add dash subcomponents receive additional parameters passed by the pa… #2819

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.

Sign up for GitHub

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

Merged
merged 8 commits into from
Apr 8, 2024
Merged

Add dash subcomponents receive additional parameters passed by the pa… #2819

Changes from 1 commit
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
ac35412
Add dash subcomponents receive additional parameters passed by the pa…
insistence Mar 28, 2024
2d08ef8
Merge branch 'plotly:dev' into insistence-dev
insistence Apr 3, 2024
656e1bc
Merge branch 'plotly:dev' into insistence-dev
insistence Apr 3, 2024
2f3762f
Update dash/dash-render/src/TreeContainer.js
insistence Apr 3, 2024
1d40f1d
Add test
insistence Apr 3, 2024
1d7cd3b
Add CHANGELOG entry
insistence Apr 3, 2024
b32687e
re-run tests
insistence Apr 7, 2024
111db6a
fix for lint
insistence Apr 7, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 12 additions & 4 deletions dash/dash-renderer/src/TreeContainer.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -237,7 +237,7 @@ class BaseTreeContainer extends Component {
);
}

getComponent(_dashprivate_layout, children, loading_state, setProps) {
getComponent(_dashprivate_layout, children, loading_state, setProps, _dashextra_controlProps) {
Copy link
Contributor

Choose a reason for hiding this comment

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

There is no need for the prefix naming _dash, I would just call it extraProps.

Copy link
Contributor Author

@insistence insistence Mar 29, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Of course, the example code repository address is https://github.com/insistence/feffery-antd-components/tree/dash-test, please use the dash-test branch. I encapsulate dash components based on Ant Design's form component, formItem component, and input component. In Ant Design, the formItem component injects relevant parameters such as value, onChange, etc. into its child elements. These parameters are crucial for completing the form validation function. There are six images below. The first three images are test cases completed based on dash2.16.1 version. It will be found that BaseTreeContainer received the relevant parameters injected by formItem, but these injected parameters have been lost in CheckedComponent, and the corresponding AntdInput does not have these parameters, so I am unable to complete the form validation function. The last three images are test cases completed based on the dash version of this PR. It will be found that BaseTreeContainer successfully received the parameters injected by formItem, and these parameters were not lost in CheckedComponent and AntdInput, so I can complete the form validation function.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

By the way, running usage.py from the example code repository above can yield the results shown in the screenshot.

Copy link
Contributor

Choose a reason for hiding this comment

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

@T4rk1n In the native Ant Design (antd) FormItem component, it's capable of automatically accessing the onChange methods of a large batch of form input child components. This enables many form-related functionalities to be implemented automatically. However, in Dash, due to the incomplete transference of additional props, similar shortcut functionalities cannot operate normally.

const {_dashprivate_config, _dashprivate_dispatch, _dashprivate_error} =
this.props;

Expand All @@ -262,7 +262,7 @@ class BaseTreeContainer extends Component {
],
_dashprivate_config
);
let props = dissoc('children', _dashprivate_layout.props);
let props = mergeRight(_dashextra_controlProps, dissoc('children', _dashprivate_layout.props));

for (let i = 0; i < childrenProps.length; i++) {
const childrenProp = childrenProps[i];
Expand Down Expand Up @@ -476,9 +476,16 @@ class BaseTreeContainer extends Component {

render() {
const {
_dashprivate_error,
_dashprivate_layout,
_dashprivate_loadingState,
_dashprivate_path
_dashprivate_loadingStateHash,
_dashprivate_path,
_dashprivate_config,
_dashprivate_dispatch,
_dashprivate_graphs,
_dashprivate_loadingMap,
..._dashextra_controlProps
Copy link
Contributor

Choose a reason for hiding this comment

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

Look like those variables are not used and fail the linting, maybe disable for that line or use ramda.without to get the rest.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you very much. This feature is very important for us to develop dash components. I would like to know if this feature will be supported after modifying the code as required, or what else do I need to do.

Copy link
Contributor

Choose a reason for hiding this comment

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

@insistence I would just like to add a test if possible and a changelog entry then it's good to go.

For the test, looking at antd Form Item code it does React.cloneElement

Could add two components in @plotly/dash-test-components like this:

import React from "react";
import PropTypes from "prop-types";

const AddPropsComponent = (props) => {
    const { children, id } = props;

    return (
        <div id={id}>
            {children.map((c, i) =>
                React.cloneElement(children, {
                    receive: `Element #${i}`,
                    id: `${id}-element-${i}`,
                })
            )}
        </div>
    );
};

AddPropsComponent.propTypes = {
    id: PropTypes.string,
    children: PropTypes.node,
};

export default AddPropsComponent;

And

import React from 'react';
import PropTypes from 'prop-types';

const ReceivePropsComponent = (props) => {
    const {id, text, receive} = props;

    return (
        <div id={id}>
            {receive || text}
        </div>
    );
}
ReceivePropsComponent.propTypes = {
    id: PropTypes.string,
    text: PropTypes.string,
    receive: PropTypes.string,
}

export default ReceivePropsComponent;

Then in a test you can check it receive the new prop.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@T4rk1n I have modified the code and added a test. Due to my unfamiliarity with dash testing, I am unsure if this test complies with dash's testing rules.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@T4rk1n This PR has passed all the checks. Is there anything else that needs to be modified in this PR?

} = this.props;

const layoutProps = this.getLayoutProps();
Expand All @@ -492,7 +499,8 @@ class BaseTreeContainer extends Component {
_dashprivate_layout,
children,
_dashprivate_loadingState,
this.setProps
this.setProps,
_dashextra_controlProps
);
}
}
Expand Down