search

plotly / dash-component-boilerplate

Get started creating your own Dash components here.
265 stars 182 forks source link

Add function components #157

Closed aeltanawy closed 9 months ago

aeltanawy commented 11 months ago

This PR addresses issue #151

I have implemented @alexcjohnson suggestion on keeping both options, class and function components, for the user.

Changes Description

Testing Done

I have ran the tests before and after my changes and got the same error report 1 passed, 2 warnings, 1 error with the same error summary ERROR tests/test_install.py::test_install - selenium.common.exceptions.WebDriverException: Message: 'chromedriver' executable needs to be in PATH.... It doesn't seem that the error I'm observing is related to changes in this PR.

I have also tried creating a boilerplate project with cookiecutter for a function component only using my local changes and the project was created without any issues. I was also able to run python usage.py for the created project and was able to update the text field interactively. More tests are required for the other component choices.

Note

I couldn't find notes on contributing so please feel free to direct me to documentation to follow and I'll update the PR accordingly.

alexcjohnson commented 10 months ago

@aeltanawy this looks great, thank you very much! My apologies for being slow to review it, we will try to give it a close look shortly.

thesct22 commented 10 months ago

Waiting for this to be merged to main 😁

aeltanawy commented 10 months ago

@T4rk1n I have updated the PR with your suggestions. Let me know if you have further comments.

aeltanawy commented 10 months ago

@T4rk1n I replaced the fragment template with functional component (same reasoning as with the async template).

aeltanawy commented 10 months ago

Actually, thinking about it now and how it may confuse the user answering the component type question even after choosing true for use_async, how about combining both questions into one and let the user choose whether to use 1- Async, 2- Function Component, 3- Class Component?

T4rk1n commented 9 months ago

Actually, thinking about it now and how it may confuse the user answering the component type question even after choosing true for use_async, how about combining both questions into one and let the user choose whether to use 1- Async, 2- Function Component, 3- Class Component?

use_async is not a component type, it can be used either with both function and class components, it is to defer the loading of resources until the component is actually used on the frontend.

aeltanawy commented 9 months ago

use_async is not a component type, it can be used either with both function and class components, it is to defer the loading of resources until the component is actually used on the frontend.

I believe you misunderstood my question. Please check my comment above regarding cookiecutter conditional prompting, as far as I know this can’t be done with the recent version as there is a standing PR that hasn’t been merged yet that addresses that.

As far as the logic goes, when the user answers true for use_async, their answer to the following question, which component to use, doesn’t take an effect (based on the if/elif/else logic of what templates to include that you suggested). If my understanding is correct then what I’m suggesting is to combine the use_async and component type questions into one, perhaps the new question can be which template to use.

T4rk1n commented 9 months ago

this can’t be done with the recent version as there is a standing PR

We don't need that feature, it's just template rendering with two variables: use_async to determine wether to defer loading of components and component_type for either a class or function component.

When use_async is True, the src/components/{{component_name}}.js is a lazy loader stub. The component code will then be located in src/fragments/{{component_name}}.js and this file need to render the component_type choice.

When use_async is False, the src/components/{{component_name}}.js is the actual component code and fragments directory is not used and deleted in the post_gen_project hook: https://github.com/plotly/dash-component-boilerplate/blob/994fb0ac07135eca7f43fd8305400b413690e20e/hooks/post_gen_project.py#L47C1-L51C1

aeltanawy commented 9 months ago

this can’t be done with the recent version as there is a standing PR

We don't need that feature, it's just template rendering with two variables: use_async to determine wether to defer loading of components and component_type for either a class or function component.

When use_async is True, the src/components/{{component_name}}.js is a lazy loader stub. The component code will then be located in src/fragments/{{component_name}}.js and this file need to render the component_type choice.

When use_async is False, the src/components/{{component_name}}.js is the actual component code and fragments directory is not used and deleted in the post_gen_project hook: https://github.com/plotly/dash-component-boilerplate/blob/994fb0ac07135eca7f43fd8305400b413690e20e/hooks/post_gen_project.py#L47C1-L51C1

Thank you for the explanation. I was suggesting a way for cookiecutter questions prompting so feel free to ignore 😊. Is there anything that needs fixing in this PR or is it ready for merging?

alexcjohnson commented 9 months ago

@aeltanawy thanks very much, this is a really nice addition!