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.

Sign up for GitHub

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

Jump to bottom

Fixes 'AttributeError' when layout is a function that returns a list of components in dash layout setter #2915

Merged
merged 6 commits into from
Aug 20, 2024
Merged

Fixes 'AttributeError' when layout is a function that returns a list of components in dash layout setter #2915

merged 6 commits into from
Aug 20, 2024

Conversation

martian0x80
Copy link
Contributor

@martian0x80 martian0x80 commented Jul 6, 2024 •
edited
Loading

Fixes #2905
Changes as the proposed solution suggested, layout setter throws an AttributeError when layout is a function that returns a list of components. The solution checks if the layout_value is an instance of list or tuple, if it is, it wraps it in a html.Div component, otherwise proceeds as usual.

Contributor Checklist

  • I have broken down my PR scope into the following TODO tasks
    • Handle the case when layout is a function that returns a list of components
  • I have run the tests locally and they passed. (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

@T4rk1n
Copy link
Contributor

T4rk1n commented Jul 9, 2024

@alexcjohnson I see this comment from a couple years ago mention this fix is mostly to empty out children for Python 2. I don't see any other uses, think we can remove the simple_clone function ?

@antonymilne antonymilne mentioned this pull request Jul 18, 2024
Open
@alexcjohnson
Copy link
Collaborator

@alexcjohnson I see this comment from a couple years ago mention this fix is mostly to empty out children for Python 2. I don't see any other uses, think we can remove the simple_clone function ?

We certainly don’t need to be maintaining any py2 code anymore, so yeah I think you’re right, let’s remove it.

T4rk1n
T4rk1n requested changes Jul 24, 2024
View reviewed changes
dash/dash.py Outdated
Comment on lines 731 to 732
if isinstance(layout_value, (list, tuple)):
layout_value = html.Div(children=layout_value)
Copy link
Contributor

Choose a reason for hiding this comment

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

We can delete the function simple_clone and assign the self.validation_layout = layout_value instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thanks, will make requested changes

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

@gvwilson gvwilson self-assigned this Jul 25, 2024
T4rk1n
T4rk1n reviewed Jul 26, 2024
View reviewed changes
Copy link
Contributor

@T4rk1n T4rk1n left a comment

Choose a reason for hiding this comment

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

Looks good, seems like there is some formatting error to fix (npm run format) then we can merge 🎉

@martian0x80
Copy link
Contributor Author

martian0x80 commented Jul 26, 2024 •
edited
Loading

@T4rk1n

Should I commit the other files that were formatted as well?
I avoided the commit hooks that invoke the black formatter with --no-verify because it wouldn't let me commit since other files that I did not change had formatting problems. After running npm run format, should I commit all the format changes or just the dash.py?

These are the changes.
image

@gvwilson gvwilson removed their assignment Aug 2, 2024
@T4rk1n
Copy link
Contributor

T4rk1n commented Aug 8, 2024

Should I commit the other files that were formatted as well?

No, I don't think it should have formatted all those files. npm probably took the wrong version of black, make sure the venv is activated before running npm run format as it may takes the black from a global env.

T4rk1n
T4rk1n approved these changes Aug 8, 2024
View reviewed changes
Copy link
Contributor

@T4rk1n T4rk1n left a comment

Choose a reason for hiding this comment

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

💃 Look good, thank you.

@martian0x80
Copy link
Contributor Author

Should I commit the other files that were formatted as well?

No, I don't think it should have formatted all those files. npm probably took the wrong version of black, make sure the venv is activated before running npm run format as it may takes the black from a global env.

Thanks, that was indeed the problem. Sorry, for being clueless.

@gvwilson gvwilson added feature something new P2 needed for current cycle fix fixes something broken and removed feature something new labels Aug 13, 2024
@T4rk1n T4rk1n merged commit c4b5540 into plotly:dev Aug 20, 2024
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@T4rk1n T4rk1n T4rk1n approved these changes

@alexcjohnson alexcjohnson Awaiting requested review from alexcjohnson alexcjohnson is a code owner

@ndrezn ndrezn Awaiting requested review from ndrezn ndrezn is a code owner

Assignees
No one assigned
Labels
fix fixes something broken P2 needed for current cycle
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Layout as list of components does not work when layout is a function
4 participants
@martian0x80 @T4rk1n @alexcjohnson @gvwilson