Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Allow strings in layout list #2900

Merged
merged 6 commits into from
Jul 4, 2024

Conversation

leeagustin
Copy link
Contributor

@leeagustin leeagustin commented Jun 22, 2024

Fixes #2890.

The problem was that strings were not allowed when the page layout was defined as a list. The issue only happens the first time—and not the second one after a refresh—because the layout validation using validate_layout() is only done the first time the page is accessed, resulting in the error dash.exceptions.NoLayoutException: List of components as layout must be a list of components only.

The solution is simply to allow strings to be included when the page layout is a list.

Contributor Checklist

  • I have broken down my PR scope into the following TODO tasks
    • Allows strings if layout is a list
  • 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

if isinstance(component, (Component,)):
_validate(component)
else:
raise exceptions.NoLayoutException(
"List of components as layout must be a list of components only."
"List of components as layout must be a list of strings and components only."
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
"List of components as layout must be a list of strings and components only."
"Only strings and components are allowed in a list layout."

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Modified in d9d963f

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 just missing a changelog entry.

@leeagustin
Copy link
Contributor Author

Looks good just missing a changelog entry.

Added in ba30a4d

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.

💃 Thank you

@leeagustin
Copy link
Contributor Author

No problem @T4rk1n! Thank you for reviewing my changes! 🙏

It seems that one of the tests failed (tests/integration/renderer/test_persistence.py:209 test_rdps005_persisted_props), although these changes shouldn't have influenced it. The test is also passing on my machine. Is this what's blocking the merge?

@T4rk1n
Copy link
Contributor

T4rk1n commented Jun 28, 2024

Is this what's blocking the merge?

Not really blocking, it's probably just a flaky test, I usually merge something else and update the branch see if it passes, if not I will mark it as flaky and then merge. Anyway it's good and will be merged before the next release.

@leeagustin
Copy link
Contributor Author

Ah okay, sounds good. Thanks for the clarification!

@ndrezn
Copy link
Member

ndrezn commented Jul 2, 2024

Nice fix!

@leeagustin
Copy link
Contributor Author

Thank you @ndrezn! 🙏

@T4rk1n
Copy link
Contributor

T4rk1n commented Jul 4, 2024

test_rdps005_persisted_props

Think there is something wrong with the CI image/container because that test fails consistently on Python 3.8 image. I will merge the PR in another branch then in dev just to be sure.

@T4rk1n T4rk1n changed the base branch from dev to merge-2900 July 4, 2024 13:51
@T4rk1n T4rk1n merged commit 121903e into plotly:merge-2900 Jul 4, 2024
1 of 3 checks passed
@T4rk1n T4rk1n mentioned this pull request Jul 4, 2024
@leeagustin leeagustin deleted the allow-strings-in-layout-list branch July 5, 2024 01:39
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

No Layout Exception when using string elements when the layout is a list
3 participants