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

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

Merged
merged 8 commits into from
Apr 8, 2024

Conversation

insistence
Copy link
Contributor

@insistence insistence commented Mar 28, 2024

When we develop dash components based on react components in Dash, some react components themselves inject additional parameters into child elements, which are very useful in many scenarios. However, when we develop this react component into a dash component, the dash component will lose this additional injection parameter. I hope to still receive this additional injected parameter in dash. Closes #2814

Copy link
Contributor

@T4rk1n T4rk1n left a comment

Choose a reason for hiding this comment

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

Can we add a test with a new test component that receives those extra props?

@@ -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

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.

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.

Comment on lines 482 to 488
_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?

Copy link
Contributor

@T4rk1n T4rk1n left a comment

Choose a reason for hiding this comment

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

💃

The new test looks really good, thank you for your contribution.

@T4rk1n T4rk1n merged commit 2ea44a5 into plotly:dev Apr 8, 2024
3 checks passed
@insistence
Copy link
Contributor Author

💃

The new test looks really good, thank you for your contribution.

Thank you for your merger. Looking forward to the next version of Dash.

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.

The problem of parameter transparency between dash custom components
3 participants