refactor(layout_columns): Now a Custom Element #931
Conversation
| const children = this.children; | ||
| writeGridClasses(this.colWidthsSpec, children, this.colUnits); |
There was a problem hiding this comment.
Currently, the children start as "grid items" already, i.e. div.bslib-grid-item.bslib-gap-spacing plus .html-fill-container if fillable = TRUE.
It's better for these items to start wrapped in the grid item containers because it reduces the impact of the custom element failing for some reason -- the markup will already be mostly correct other than the column width classes.
On the other hand, the children could be bare children that are promoted to grid items, if not already. This would make it easier to use <bslib-layout-columns> in contexts where it isn't reasonable to add the grid item container, as well as dynamically adding new contents to the layout (which we don't currently support but could in the future).
There was a problem hiding this comment.
I don't think we need to do this now. In fact, I adjusted our CSS to not rely on the .bslib-grid-item class at all: 425ecfb
| // fill in `md` breakpoint if only large breakpoints are specified | ||
| const hasLg = ["lg", "xl", "xxl"].some((size) => colWidths.has(size)); | ||
| const hasSm = ["sm", "md"].some((size) => colWidths.has(size)); | ||
|
|
||
| if (hasLg && !hasSm) { | ||
| const smallest = ["lg", "xl", "xxl"].filter((size) => | ||
| colWidths.has(size) | ||
| ); | ||
| colWidths.set("md", colWidths.get(smallest[0])); | ||
| } |
There was a problem hiding this comment.
I think we should do this via the .bslib-grid-item class, e.g. using a default width of 6 column units or working out from --bs-columns.
There was a problem hiding this comment.
I updated this to use a fallback span of 6, following a CSS variable that's customizable on the .bslib-grid parent element. We tweak this as necessary when the auto-fit algorithm adjusts the --bs-columns variable.
Hides the element until the colwidth spec is applied. Not used yet.
| grid_item_container <- function(el, ..., fillable = TRUE) { | ||
| div( | ||
| ..., | ||
| class = "bslib-gap-spacing", | ||
| class = "bslib-grid-item bslib-gap-spacing", |
There was a problem hiding this comment.
I think I'd be in support for dropping this if aren't adding rules/logic for it?
| class = "bslib-grid-item bslib-gap-spacing", | |
| class = "bslib-gap-spacing", |
There was a problem hiding this comment.
I'm okay with taking it out, but I do like that it documents the object and gives the user a hook. Yes, they could .bslib-grid > * but as a user this always feels brittle, so I have a slight preference for including a stable selector even if we don't use it directly.
| if (bk.some((val) => val === 0)) { | ||
| throw new Error( | ||
| "Column values must be greater than 0 to indicate width, or negative to indicate a column offset." | ||
| ); | ||
| } | ||
|
|
||
| if (bk.length > 1 && bk.some((val) => isNaN(val))) { | ||
| throw new Error( | ||
| "Cannot mix widths and missing values. All column widths must be specified, or choose auto widths using a single `null` value." | ||
| ); | ||
| } |
There was a problem hiding this comment.
I can see this mirrors some of the same validation we do prior to markup generation, can maybe get rid of it?
There was a problem hiding this comment.
The reason I kept these is that this component could also, in theory and maybe even in practice, work in Quarto (or anywhere else really), without a Python or R engine. In that case, these checks would be helpful.
| // which we signal with cursor < 0 | ||
| newCursor = isEmpty ? -1 * (newCursor % unitsRow) : incr; | ||
| } | ||
| // console.log(`cursor: ${cursor} -> ${newCursor} (+${incr}${isEmpty ? " (empty)" : ""})`); |
There was a problem hiding this comment.
| // console.log(`cursor: ${cursor} -> ${newCursor} (+${incr}${isEmpty ? " (empty)" : ""})`); |
There was a problem hiding this comment.
I intentionally kept this comment. If our future selves need to debug this code, this is the most succinct way to get a sense of how this whole thing works. That felt non-obvious enough that I think it's worth keeping the comment for later.
NEWS.md
Outdated
| * `layout_columns()` was rewritten in Typescript as a custom element to improve the portability of the component. (#931) | ||
|
|
||
| * When `layout_columns()` is given a `row_heights` value that is not a `breakpoints()` object, that value is used for the row heights at all breakpoints. Previously, it was used for the row heights from `"sm"` up. (#931) | ||
|
|
||
| * When `layout_columns()` is given a `col_widths` value with `breakpoints()` at | ||
| `lg` or wider, it now uses a better default column width for the smaller | ||
| breakpoints not listed in the `col_widths` value. That said, you can always | ||
| include `sm` or `md` in your `breakpoints()` definition to have complete | ||
| control over column widths at those sizes. (#931) | ||
|
|
There was a problem hiding this comment.
It might be worth making a high-level point that the internals of layout_columns() was refactored to be a custom element, then list the other things a sub-items that came out of that work?
There was a problem hiding this comment.
My thinking here is that the row_heights and col_widths changes were basically orthogonal to the custom element change. They're really separate things we decided to do while working on the Typescript port.
| * `layout_columns()` was rewritten in Typescript as a custom element to improve the portability of the component. (#931) | |
| * When `layout_columns()` is given a `row_heights` value that is not a `breakpoints()` object, that value is used for the row heights at all breakpoints. Previously, it was used for the row heights from `"sm"` up. (#931) | |
| * When `layout_columns()` is given a `col_widths` value with `breakpoints()` at | |
| `lg` or wider, it now uses a better default column width for the smaller | |
| breakpoints not listed in the `col_widths` value. That said, you can always | |
| include `sm` or `md` in your `breakpoints()` definition to have complete | |
| control over column widths at those sizes. (#931) | |
| * `layout_columns()` was rewritten in Typescript as a custom element to improve the portability of the component. (#931) | |
| * When `layout_columns()` is given a `row_heights` value that is not a `breakpoints()` object, that value is used for the row heights at all breakpoints. Previously, it was used for the row heights from `"sm"` up. (#931) | |
| * When `layout_columns()` is given a `col_widths` value with `breakpoints()` at `lg` or wider, it now uses a better default column width for the smaller breakpoints not listed in the `col_widths` value. That said, you can always include `sm` or `md` in your `breakpoints()` definition to have complete control over column widths at those sizes. (#931) | |
This PR replaces the R code in
layout_columns()with a new<bslib-layout-columns>custom element. All of the column width logic is now in Typescript and happens on the client rather than server-side.