Skip to content

Conversation

@cpsievert
Copy link
Collaborator

@cpsievert cpsievert commented Jan 9, 2024

Closes #948

@cpsievert cpsievert requested a review from schloerke January 9, 2024 02:04
@wch
Copy link
Collaborator

wch commented Jan 9, 2024

This code reaches into context-manager children in a way that's very specific to this one use case and also feels a bit brittle. I think this actually points to a more general issue that we're going to have to deal with.

Currently, in Shiny Core we have two stages of dealing with the tree of components:

  • Building the tree with nested function calls. At this point, the tree may consist of strings, Tags, and Tagifiable objects (and a few other types which I'll ignore for this discussion).
  • Calling .tagify() on the top-level node recurses through the tree and converts all nodes to strings, and Tags.

With Shiny Express, there's another type of object that can be in the tree: RecallContextManagers. These are actually Tagifiable, but the problem is that some functions, like card(), want to check if their children are a specific class, like CardItem, before deciding what to do with them. This happens before the .tagify() step. I think we'll need a generalizable way of handling RecallContextManagers, because this issue will likely crop up in other places.

Maybe the steps should be something like this:

  • Build the tree with strings, Tags, Tagifiables, and RecallContextManagers. As the tree is being built, functions can
    check if their children are RecallContextManagers, and if so, call a method.resolve(), which looks like this:
    def resolve(self):
        return self.fn(*self.args, **self.kwargs)
    This would only be necessary to do in functions like card_body() which check if their children are objects of specific classes (that are not Tag).

@wch wch added this to the v0.7.0 milestone Jan 9, 2024
@wch wch mentioned this pull request Jan 9, 2024
55 tasks
@wch
Copy link
Collaborator

wch commented Jan 11, 2024

Closing because this was superseded by #992.

@wch wch closed this Jan 11, 2024
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.

shiny.express.ui.card doesn't render card items properly

3 participants