WIP: Tool calling UI #52
Conversation
* `display` can be markdown or HTML * `display` can be a list with `html`, `markdown` or `text` used in that order * `display_tool_request` hides the tool request if FALSE
| <button | ||
| class="card-header" | ||
| id="${headerId}" | ||
| @click="${this.#toggleCollapse}" | ||
| aria-expanded="${this.expanded}" | ||
| aria-controls="${contentId}" | ||
| > |
There was a problem hiding this comment.
This is nice -- maybe worth bringing to bslib as well (not now, but eventually)?
There was a problem hiding this comment.
Something to consider adding when we turn bslib::card() into a custom component 😉
|
|
||
| return html` | ||
| <div | ||
| class="shiny-tool-card card bslib-card bslib-mb-spacing html-fill-item html-fill-container m-0" |
There was a problem hiding this comment.
This is maybe a little too aggressive in removing classes, but seems like we don't need filling layout, and also don't need bslib's complex margin/spacing rules?
| class="shiny-tool-card card bslib-card bslib-mb-spacing html-fill-item html-fill-container m-0" | |
| class="shiny-tool-card card m-0" |
There was a problem hiding this comment.
Yeah, I was thinking we do want filling -- if you return only HTML and turn off tool request display, you could have your tool return a map or plot that fills the card. I have a demo planned to show that.
There was a problem hiding this comment.
And just generally the further we get away from bslib::card() markup, the more space we leave for updates to bslib::card() to miss the nuance of differences in this component. Sounds like a bad time waiting to happen.
There was a problem hiding this comment.
Ahh OK, I had missed the max-height setting on .shiny-tool-card
There was a problem hiding this comment.
It does seem worth dropping bslib-mb-spacing and bslib-gap-spacing though since we're in control of the container of the card?
There was a problem hiding this comment.
No, unless you can think of a way this would cause problems. My take is that we should follow bslib::card() as close as reasonably possible unless we have evidence that including a class is going to cause issues.
| <div class="collapse-indicator">${unsafeHTML(ICONS.plus)}</div> | ||
| </button> | ||
| <div | ||
| class="card-body bslib-gap-spacing html-fill-item html-fill-container${this |
There was a problem hiding this comment.
Assuming we don't actually need filling layout or complex gap spacing
| class="card-body bslib-gap-spacing html-fill-item html-fill-container${this | |
| class="card-body ${this |
| @property({ type: Boolean, attribute: "show-request" }) | ||
| showRequest = true |
There was a problem hiding this comment.
It'd be good to document that what we mean here is showing the request inside the result container (i.e., the request container element should always be hidden).
pkg-r/R/contents_shinychat.R
Outdated
| html <- list( | ||
| value = format(display), | ||
| value_type = "html", | ||
| deps = htmltools::findDependencies(display) | ||
| ) | ||
| return(html) |
There was a problem hiding this comment.
I think you could get away with just returning value and value_type from this function?
| html <- list( | |
| value = format(display), | |
| value_type = "html", | |
| deps = htmltools::findDependencies(display) | |
| ) | |
| return(html) | |
| return(list(value = display, value_type = "html")) |
There was a problem hiding this comment.
Going even further, seems the logic could be cleaner if you had one function to just get the value_type, and another function that uses it, but returns the value.
There was a problem hiding this comment.
There's some overlapping logic between value_type and value that doesn't make it worth separating into two functions. But I was able to clean this up quite a bit with a second pass in f3c28fa
TODO
+or-depending on state (with a neat animation)