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 support for adding "extra components" to an app's layout #1700

Merged
merged 3 commits into from
Aug 16, 2021

Conversation

jonmmease
Copy link
Contributor

This PR is a foundational part of the coming long_callback PR, which will bring the long_callback decorator from Dash Labs to Dash. I think it may be useful in other contexts as well, so I thought I'd split it off into a separate PR for discussion.

I added a new private _extra_components property to dash.Dash app instances that is initialized to an empty list. When components are added to this list, they are appended to the layout returned by _layout_value.

The motivation is to make it possible for long_callback to add Interval and Store components to the layout without returning them to the user and requiring the user to position them in the layout themselves.

These are components that are automatically appended to the app's layout
@jonmmease
Copy link
Contributor Author

Not sure what's going on with Percy 🤔

@jonmmease jonmmease mentioned this pull request Aug 7, 2021
3 tasks
@jonmmease jonmmease closed this Aug 12, 2021
@jonmmease jonmmease reopened this Aug 12, 2021
@jonmmease
Copy link
Contributor Author

all green now. Ready for a look when you have a chance @alexcjohnson

dash/dash.py Outdated

# Add extra hidden components
if self._extra_components:
if not hasattr(layout, "children"):
Copy link
Collaborator

Choose a reason for hiding this comment

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

Some simple apps just use app.layout = DataTable(...) or a graph or something, that don't support children. Mostly this is just for testing purposes but maybe not always. In that case what do we do, throw an error? Wrap in html.Div? Actually there's something nice about wrapping in a div, then we don't need to keep testing if we've already added these components into the layout, and don't need to mutate it. I can't really think of how mutating the layout would cause a problem but it makes me uneasy.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can you think of any cases where wrapping in a Div would break something? Any concern if the top-level is a ddk.App or dbc.App?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think this would cause a problem - we're already inside a div from the react entry point

dash/dash/dash.py

Lines 86 to 92 in a19c274

_app_entry = """
<div id="react-entry-point">
<div class="_dash-loading">
Loading...
</div>
</div>
"""

Could be nice at some point to allow the top level layout to be an array, but this would either also require wrapping in a div or would require changes to the renderer.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, wrapping in a div seems like the best solution then. Thanks!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done in b88d8a3

Copy link
Collaborator

@alexcjohnson alexcjohnson left a comment

Choose a reason for hiding this comment

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

LGTM! 💃 after adding a changelog entry.

@jonmmease
Copy link
Contributor Author

...after adding a changelog entry.

This isn't a public feature (at least not yet), so I wasn't picturing adding a changelog for it by itself.

I'll go ahead and merge this one so that I can fix up the conflicts in #1702, but I can add something more to the changelog over in that PR if you'd still like to mention this PR on its own.

Thanks!

@jonmmease jonmmease merged commit e277be5 into dev Aug 16, 2021
@alexcjohnson
Copy link
Collaborator

Ok, that makes sense

@archmoj archmoj deleted the hidden_components branch January 27, 2022 16:51
@archmoj archmoj restored the hidden_components branch January 27, 2022 16:51
@archmoj archmoj deleted the hidden_components branch January 27, 2022 16:54
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.

2 participants