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

Updated Pack properties to use new Travertino dataclass-based API #2443

Draft
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

HalfWhitt
Copy link
Contributor

This has been tested against the up-to-date Travertino with beeware/travertino#141, but can't be merged here until there's a new release of Travertino and Toga's dependency is updated.

PR Checklist:

  • All new features have been tested
  • All new features have been documented
  • I have read the CONTRIBUTING.md file
  • I will abide by the code of conduct

@freakboy3742
Copy link
Member

@HalfWhitt I don't know if you've got any plans for more Travertino changes; the only thing that comes to mind for me was the issue about py3.13 support that I flagged during the review of the data class PR.

Once you're done, let me know and I'll cut a release to clear the path for this PR.

@HalfWhitt
Copy link
Contributor Author

@freakboy3742 Well... I have started converting the rest of the tests to pytest. But that shouldn't affect anything outward-facing. Up to you whether you'd rather wait till that's done or not.

@HalfWhitt
Copy link
Contributor Author

HalfWhitt commented Mar 6, 2024

On second thought, I suppose this would be a good time to see about implementing list_property and/or composite_property, assuming those are still intended to happen. I only know about them from the commented-out sections. It looks like composite_property is supposed to function something like how the font parser already does, splitting a string into the relevant aliased properties, is that correct?

@freakboy3742
Copy link
Member

On second thought, I suppose this would be a good time to see about implementing list_property and/or composite_property, assuming those are still intended to happen. I only know about them from the commented-out sections. It looks like composite_property is supposed to function something like how the font parser already does, splitting a string into the relevant aliased properties, is that correct?

That's correct. composite_property would be used by a font property, and list_property by font_family - with the obvious complication that font_family is then the first part of `font.

Those two definitions have been skipped because Pack has been able to survive without them - for the purposes of a "CSS-lite", it's fine to require the more explicit font syntax. They've only been stubbed out because they will be required for Colosseum.

I guess the question about whether to wait or not is up to you. They're not super-high priorities for me personally, because there's a totally acceptable workaround - but it they motivate you, then it's probably worth waiting so the next Travertino release is a really juicy one. And if someone (cough) were to use this as the start of getting Colosseum back on track... I also would not complain :-)

@HalfWhitt
Copy link
Contributor Author

Alright yeah, hold off for now, I think I'll try my hand at them. And no promises about Colosseum, but I had in fact been thinking of such a thing...

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.

2 participants