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

Add title and sub-title to screens. #3199

Merged
merged 7 commits into from
Sep 11, 2023
Merged

Conversation

rodrigogiraoserrao
Copy link
Contributor

Mimicking 'App', we provide class variables TITLE and SUB_TITLE for the screen defaults and those can then be changed via the title and sub_title reactive attributes.

Related issue: #3195

Mimicking 'App', we provide class variables TITLE and SUB_TITLE for the screen defaults and those can then be changed via the title and sub_title reactive attributes.

Related issue: #3195
@rodrigogiraoserrao rodrigogiraoserrao marked this pull request as ready for review August 29, 2023 11:10
@rodrigogiraoserrao rodrigogiraoserrao self-assigned this Aug 29, 2023
@rodrigogiraoserrao rodrigogiraoserrao added the enhancement New feature or request label Aug 29, 2023
@rodrigogiraoserrao rodrigogiraoserrao linked an issue Aug 29, 2023 that may be closed by this pull request
@@ -128,10 +129,31 @@ class Screen(Generic[ScreenResultType], Widget):
background: $surface;
}
"""

TITLE: str | None = None
Copy link
Collaborator

Choose a reason for hiding this comment

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

These should be typed as ClassVar

Copy link
Contributor Author

Choose a reason for hiding this comment

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

See 5ec3fea.

@@ -161,11 +161,19 @@ def _on_click(self):
self.toggle_class("-tall")

def _on_mount(self, _: Mount) -> None:
def set_title(title: str) -> None:
def set_title() -> None:
screen_title = self.screen.title
Copy link
Collaborator

Choose a reason for hiding this comment

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

We should have a more elegant way of exposing the actual title. So that a developer doesn't need to replicate this logic.

Perhaps a computed value called screen_title and screen_subtitle

Copy link
Contributor Author

Choose a reason for hiding this comment

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

See 4ed93d4

@@ -160,12 +160,28 @@ def watch_tall(self, tall: bool) -> None:
def _on_click(self):
self.toggle_class("-tall")

@property
def screen_title(self) -> str:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Docstrings pls.

Could we make these computed reactives? That way you could watch self.screen_title

Copy link
Contributor Author

Choose a reason for hiding this comment

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

See 5a15e9c for docstrings.
As per our conversation, we can't make them computed reactives because we can't trigger the compute method when the reactives on Screen or App change.

@rodrigogiraoserrao rodrigogiraoserrao merged commit 7d4a47b into main Sep 11, 2023
@rodrigogiraoserrao rodrigogiraoserrao deleted the screen-title-sub-title branch September 11, 2023 12:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Screens should have a title and subtitle
2 participants