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

max-height: 100% with a height: auto container causes child widget to disappear #2975

Closed
davep opened this issue Jul 19, 2023 · 5 comments · Fixed by #3758 or #3814
Closed

max-height: 100% with a height: auto container causes child widget to disappear #2975

davep opened this issue Jul 19, 2023 · 5 comments · Fixed by #3758 or #3814
Assignees
Labels
enhancement New feature or request Task

Comments

@davep
Copy link
Contributor

davep commented Jul 19, 2023

Given this code:

from textual.app import App, ComposeResult
from textual.widgets import DataTable, Static

class HappyDataTableFunApp(App[None]):

    CSS = """
    DataTable {
        max-height: 100%;
    }
    """

    def populate(self, table: DataTable) -> DataTable:
        for n in range(20):
            table.add_column(f"Column {n}")
        for row in range(1000):
            table.add_row(
                *[str(row * n) for n in range(20)]
            )
        return table

    def compose(self) -> ComposeResult:
        with Static():
            yield self.populate(DataTable())

if __name__ == "__main__":
    HappyDataTableFunApp().run()

the DataTable doesn't show. This seems to be the combination of a child widget that is height: auto; and max-height: 100%; with a parent container that is height: auto.

More generally the issue seems to be that if a child widget is height: auto and max-height: 100% and the parent container is height: auto there's no way to work out a height.

Perhaps there's some scope here to settle on a less astonishing outcome?

@davep davep added enhancement New feature or request Task labels Jul 19, 2023
@willmcgugan
Copy link
Collaborator

It is less to do with styling, and more to do with the fact that Static was not designed to be a container. It doesn't make sense to allow adding children, even though the context manager allows it.

I think the only resolution would be to make this an error. i.e. something like raise NotAContainer("Can't add children to a Static").

Suggest we are fairly cautious about what we add this to. Some widgets may not be containers by default, but could be styled to be containers.

Let @willmcgugan know if this is more than half a day's work.

@davep
Copy link
Contributor Author

davep commented Nov 22, 2023

Somewhat related, because I see folk using Static as a container quite often, perhaps this wording:

Can be used for Rich renderables and can also be the base for other types of widgets.

in the the docs for Static could do with a wee revamp, as I suspect it has caused people to think it should be used as a container.

Also, the tutorial uses Static as the base class for a compound widget (which now I think about it might account for why I see people do that so often).

@TomJGooding
Copy link
Contributor

Yup, I can confirm this is what I was doing too when I was new to Textual based on the docs!

The specific mention of Static in the widgets guide to "create custom widgets of your own" might also be compounding this confusion: https://textual.textualize.io/guide/widgets/#static-widget

Copy link

Don't forget to star the repository!

Follow @textualizeio for Textual updates.

Copy link

github-actions bot commented Dec 7, 2023

Don't forget to star the repository!

Follow @textualizeio for Textual updates.

TomJGooding added a commit to TomJGooding/textual that referenced this issue Mar 14, 2024
The `max-height` of the `DataTable` was changed to 100vh rather than
100% in Textualize#3566, because at the time this caused issues with auto height
containers, as described in Textualize#2975.

However this issue was later fixed in Textualize#3814. This PR changes the
`max-height` of the `DataTable` back to 100%, as 100vh will actually
break common layouts and seems no longer required to workaround this
auto height issue.

Fixes Textualize#4286.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request Task
Projects
None yet
3 participants