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

changed typecheck for list value to use isinstance instead of type #3108

Open
wants to merge 10 commits into
base: dev
Choose a base branch
from

Conversation

kenshima4
Copy link

@kenshima4 kenshima4 commented Dec 17, 2024

Solves: Layout as list of components does not work with Dash Pages page_container #2924

I've checked if the values passed to layout setter isinstance list, if so, set the values to html.Div (dash Component)

Contributor Checklist

  • I have broken down my PR scope into the following TODO tasks
    • Set values passed as list to html.Div
    • Changed typecheck to use isintance instead of type
  • I have run the tests locally and they passed (based on automated tests). (refer to testing section in contributing)
  • I have added tests, or extended existing tests, to cover any new features or bugs fixed in this PR

optionals

  • I have added entry in the CHANGELOG.md
  • If this PR needs a follow-up in dash docs, community thread, I have mentioned the relevant URLS as follows
    • this GitHub #PR number updates the dash docs
    • here is the show and tell thread in Plotly Dash community

Sorry, something went wrong.

@gvwilson
Copy link
Contributor

gvwilson commented Jan 3, 2025

@T4rk1n this one looks good to me, but can you please check it before we merge?

@kenshima4
Copy link
Author

kenshima4 commented Jan 3, 2025

Hi @gvwilson @T4rk1n, will fix the percy, testing etc this weekend. Will revert back.

@T4rk1n
Copy link
Contributor

T4rk1n commented Jan 6, 2025

I'd rather see the check fixed than change the layout. The check at

dash/dash/dash.py

Line 2257 in bbd013c

if _ID_CONTENT not in self.validation_layout:

use a brittle/unoptimal API to check for ids that is outdated (doesn't work with component as props).

for t in self._traverse_ids():

The issue here is that we end up with a list of list so the traverse can't find it here:

dash/dash/dash.py

Lines 2250 to 2255 in bbd013c

+ [
# pylint: disable=not-callable
self.layout()
if callable(self.layout)
else self.layout
]

Need to extract the layout value then concat directly it if it's a list if not wrap in a list.

…s id content check
@kenshima4
Copy link
Author

kenshima4 commented Jan 9, 2025

Hi @T4rk1n, I made the changes you recommended, please check. I'd just like to ask if all the integration tests need to remain unchanged or would I need to change them based on my changes?

Because I see even on dev branch the integration tests are failing (unless I'm wrong).
Can check with pytest -k test_cbcx005_grouped_clicks from dash directory.

Is this correct, or can I go ahead and modify these failing integration tests?

@kenshima4 kenshima4 force-pushed the layout-accept-lists-as-component branch from 8e14147 to ad51501 Compare February 10, 2025 15:31
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.

None yet

4 participants