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

Screen css #2744

Merged
merged 8 commits into from
Jun 12, 2023
Merged

Screen css #2744

merged 8 commits into from
Jun 12, 2023

Conversation

rodrigogiraoserrao
Copy link
Contributor

Closes #2137.
Adds CSS and CSS_PATH class variables to screens.

  • Docstrings on all new or modified functions / classes
  • Updated documentation
  • Updated CHANGELOG.md (where appropriate)

@willmcgugan inside App._load_screen_css, why don't I traverse all the rules that were read and dynamically add a CSS selector to match the screen so that screen CSS only matches the screen it was imported from?

Copy link
Collaborator

@willmcgugan willmcgugan left a comment

Choose a reason for hiding this comment

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

If you were to push and pop the same screen multiple times, will it parse the CSS each time?

src/textual/_path.py Show resolved Hide resolved
src/textual/screen.py Outdated Show resolved Hide resolved
@rodrigogiraoserrao
Copy link
Contributor Author

If you were to push and pop the same screen multiple times, will it parse the CSS each time?

No. That's why there is a _css_screens_loaded set and we check it before calling _load_screen_css.

What do you think of this:

inside App._load_screen_css, why don't I traverse all the rules that were read and dynamically add a CSS selector to match the screen so that screen CSS only matches the screen it was imported from?

@willmcgugan
Copy link
Collaborator

If you were to push and pop the same screen multiple times, will it parse the CSS each time?

No. That's why there is a _css_screens_loaded set and we check it before calling _load_screen_css.

That's a weak set, right? Does that not mean that as soon as you pop the screen (and it goes out of scope), it will forget that it parsed the CSS.

This may not be a problem, just wanted to understand.

What do you think of this:

inside App._load_screen_css, why don't I traverse all the rules that were read and dynamically add a CSS selector to match the screen so that screen CSS only matches the screen it was imported from?

Sounds like it may be worth exploring. Do you have an idea how you would implement it?

@rodrigogiraoserrao
Copy link
Contributor Author

rodrigogiraoserrao commented Jun 7, 2023

Sounds like it may be worth exploring. Do you have an idea how you would implement it?

I'd load and parse the styles into a separate stylesheet.
Then I'd traverse the rules there and modify the selectors to include the screen in the beginning.
Then, I'd add the new stylesheet to the app's stylesheet.

Do not keep an explicit registry of screens whose CSS has been parsed. Instead, grow methods in the file monitor and in the stylesheet to check if a given path is already being monitored/sourced and add the paths if it isn't.
@willmcgugan
Copy link
Collaborator

Sounds like it may be worth exploring. Do you have an idea how you would implement it?

I'd load and parse the styles into a separate stylesheet. Then I'd traverse the rules there and modify the selectors to include the screen in the beginning. Then, I'd add the new stylesheet to the app's stylesheet.

Clever. Lets consider that for a future update.

"""File paths to load CSS from.

Note:
This CSS applies to the whole app.
Copy link
Member

Choose a reason for hiding this comment

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

This is such a huge footgun :(

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I know. I wanted to try and restrict this via this suggestion but I don't think I'm supposed to go ahead and do it now... :(

Choose a reason for hiding this comment

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

I just shot myself in the foot with this footgun.


counter = 0

def reparse_wrapper(reparse):
Copy link
Member

Choose a reason for hiding this comment

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

Nice 👍

Copy link
Member

@darrenburns darrenburns left a comment

Choose a reason for hiding this comment

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

This looks good to me. Small suggestion: this feels like a good candidate for a snapshot test which could make sure that screen CSS and CSS_PATH works and the precedence is as expected.

@rodrigogiraoserrao
Copy link
Contributor Author

This looks good to me. Small suggestion: this feels like a good candidate for a snapshot test which could make sure that screen CSS and CSS_PATH works and the precedence is as expected.

I don't understand your suggestion. I have tests that check that the styles imported from the screen CSS and CSS_PATH variables are applied and with the correct precedence.
Are you saying that those would be better as snapshot tests?

@darrenburns
Copy link
Member

Possibly, but you don't need to change them now. When I see tests that query the DOM and check styles it feels like it's trying to programmatically do something that snapshot tests give you for free.

@rodrigogiraoserrao
Copy link
Contributor Author

Possibly, but you don't need to change them now. When I see tests that query the DOM and check styles it feels like it's trying to programmatically do something that snapshot tests give you for free.

Ahhh ok.
When I can, I do things programmatically because it is faster than running a snapshot test. (Right?)
Also, I had multiple variations of pretty much the same thing, so that's why I thought code was better 🤷
Thanks for your input!

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.

Add CSS to Screens
4 participants