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

  • [x ] 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

  • (will leave this for when I've got more experience)
  • 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

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

.gitignore Outdated
Comment on lines 95 to 96
app.py
launch.json
Copy link
Member

Choose a reason for hiding this comment

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

I'd recommend just avoiding committing these files rather than changing .gitignore; while unlikely we may need these file names in the future.

Copy link
Author

@kenshima4 kenshima4 Jan 9, 2025

Choose a reason for hiding this comment

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

Hi @ndrezn will remove them from gitignore and make sure not to commit the files. Thank you.

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

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.

4 participants