feat(page_main_container): Remove fill container and min-height/width when fillable=FALSE

[gadenbuie]

• page_main_container() is only a fill carrier when fillable = TRUE. Or more specifically, it should only be a fill container when fillable is TRUE.
• Only apply min-height/width when the main container is fillable.

library(shiny)
pkgload::load_all()
library(shinychat)

ui <- page_sidebar(
  actionButton("btn", "Button"),
  chat_ui("chat", fill = TRUE),
  fillable = TRUE
)

# ui <- page_navbar(
#   fillable = FALSE,
#   sidebar = sidebar(title = "Sidebar"),
#   nav_panel("one", actionButton("btn", "Button"), chat_ui("chat", fill = TRUE))
# )

server <- function(input, output, session) {
  observeEvent(input$chat_user_input, {
    # In a real app, this would call out to a chat model or API,
    # perhaps using the 'elmer' package.
    response <- paste0(
      "You said:\n\n",
      "<blockquote>",
      htmltools::htmlEscape(input$chat_user_input),
      "</blockquote>"
    )
    chat_append("chat", response)
  })
}

shinyApp(ui, server)

[cpsievert]

I'm probably missing something here, but if the parent of bslib-page-main is not fillable, then AFAIK this doesn't need to be a fill item?

Suggested change

	item = TRUE,
	item = fillable,

[gadenbuie]

Maybe not? But I know for sure that, without having to consider parent containers, we want to break fillability for children by making sure the container isn't a fill container. We could probably also set item = fillable, but then we have to consider all of the parent layouts. page_main_container() isn't just used in page_sidebar(), it's also used in navbar_page() with a sidebar where the DOM structure is quite different from page_sidebar().

I know for certain that removing html-fill-container does what we want it to do in this case and that that's correct in all layouts. That's also why I used .html-fill-container for min-height/min-width above. My general take is that .html-fill-item is upward-indicating, as in it makes the child filling when the parent context is fillable. .html-fill-container is downward-facing and communicates intent downward to its children.

My take is that we could probably either make .bslib-page-main a fill carrier when fillable = TRUE or add no filling properties when fillable = FALSE, but we could certainly work with the fill container property. So this choice mostly came down to 1) proving we want container = fillable is easy; 2) proving we want item = fillable requires enumerating all the layouts and making sure that's right.

[cpsievert]

Ah ok, I was missing that navbar_page() context, thanks for writing that out. Although I'll probably find the code confusing to revisit, I like the careful approach your taking. Maybe consider linking to your comment here in the code?

[gadenbuie]

Well, in the mean time I convinced myself that we should go wholesale as_fill_carrier() when fillable=TRUE and just wrap things in the <main> tag otherwise.

I think the reason I used as_fill_carrier() was in case we use page_main_container() in other situations; the fact that that also adds display: flex was the unintended side-effect.

[cpsievert]

Please also add a NEWS.md, thanks!