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

Added Container widget. #11

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

the-allanc
Copy link

This wraps an existing widget, but allows for the wrapped widget to be swapped out for another (or even removed).

@philer
Copy link
Owner

philer commented Mar 31, 2023

Hey, thanks for your PR!

I like the idea of conditional rendering via composable widgets. However, I'm not quite sure how your Container could be used. I tried refactoring the existing Drive widget, which already does conditional rendering, but it turned out to be quite complicated.
Could you add a demonstration, preferably in the /examples directory?

While thinking about your code I also found some possible simplifications for the existing :layout methods. You may need to adjust your own Container:layout method accordingly. Basically container widgets no longer insert themselves into the returned children list but always include their direct children. Here is the relevant commit: ecac1fe

@the-allanc
Copy link
Author

I've rebased off your master branch, and added an example based roughly on the sort of thing that I'm using Polycore for.

I've not yet looked at the code to see whether it needs some changes relating to your changes to dealing with children widgets in layout methods.

Allan Crooks added 2 commits April 8, 2023 22:56
This wraps an existing widget, but allows for the wrapped widget
to be swapped out for another (or even removed).
@the-allanc
Copy link
Author

I've updated my merge request, as the setting of background colours wasn't working initially after I rebased.

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