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 index by id #203

Merged
merged 6 commits into from
Jul 11, 2023
Merged

add index by id #203

merged 6 commits into from
Jul 11, 2023

Conversation

BeastyBlacksmith
Copy link
Contributor

@BeastyBlacksmith BeastyBlacksmith commented Jun 7, 2023

This adds the ability to index into DashApp, Components and collections of Components by id. Like

using Dash
myapp.app["my-id"]

This is mainly useful for testing purposes and a convenience feature.

@BeastyBlacksmith BeastyBlacksmith marked this pull request as ready for review June 7, 2023 19:00
@etpinard
Copy link
Collaborator

Hi @BeastyBlacksmith - nice to see someone making a feature PR to Dash.jl !

Does the python version of dash expose something similar from its public API or its testing API?

If so, I think we could safely merge your PR.

If not, I (not a Plotly employee, I'm just a Dash.jl user with write rights to this repo) wouldn't be comfortable making the Dash.jl API diverge from the python version of dash.

@alexcjohnson
Copy link
Contributor

I don't think it gets a lot of use, but Python Dash does allow you to find components within any component tree by ID:

>>> from dash import html
>>> container = html.Div([html.Div(id="a"), html.Div(id="b")])
>>> container['a']
Div(id='a')

It's a little funny to me that what you have here is app[id] though, would it make sense to change to app.layout[id]?

@BeastyBlacksmith
Copy link
Contributor Author

It's a little funny to me that what you have here is app[id] though, would it make sense to change to app.layout[id]?

They work both

@BeastyBlacksmith
Copy link
Contributor Author

bump.

They work both

Should I add a test for this?

@etpinard
Copy link
Collaborator

Hi @BeastyBlacksmith, if you were to remove the Base.getindex(app::Dash.DashApp, id::AbstractString) method while keeping functionally for app.layout[id], I could merge this PR and include it into Dash.jl v1.3.0 that I'd like to release at some point this week.

@BeastyBlacksmith
Copy link
Contributor Author

I don't think it does any harm, but I removed it.

@etpinard
Copy link
Collaborator

Thanks for the PR @BeastyBlacksmith - merging !

@etpinard etpinard merged commit 942d633 into plotly:dev Jul 11, 2023
@etpinard etpinard mentioned this pull request Jul 12, 2023
@BeastyBlacksmith BeastyBlacksmith deleted the bbs/index-by-id branch July 28, 2023 18:15
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.

3 participants