KingOfTac commented 8 months ago

Pull Request

📖 Description

~~This PR is a first step towards better extensibility for DataGrid by making it possible to override the row's method it uses to apply the grid's column definitions to the row.~~

I ended up removing all the inline styling logic from DataGrid and its sub components. Now that more modern CSS Grid capabilities are available since DataGrid's original implementation we can use less JS for applying the grid's layout across rows and cells in favor of these newer Grid and FAST features, namely SubGrid and fast-element's CSS binding capabilities.

Prior to this change DataGrid would either auto calculate the string for the rows' grid-template-columns property which they would apply as an inline style, or DataGrid would accept a manual value for the grid columns via its grid-template-columns attribute.

After this change the grid-template-columns attribute is removed from DataGrid along with all of the logic for creating the CSS string for the columns and rows and cells no longer apply inline styles to themselves for layout within the grid.

The CSS layout for DataGrid is now something that needs to be applied by authors consuming Foundation. This ends up making DataGrid easier to style with custom layouts because extending the base class is no longer a requirement to override the default layout functionality and can instead be done entirely through styles.

Breaking changes

Data Grid's grid-template-columns attribute is removed.
Data Grid Row's grid-template-columns attribute is removed.
Data Grid no longer generates the CSS string for grid-template-columns.
Data Grid Row no longer applies the grid-template-columns as an inline style.
Data Grid Cell no longer applies grid-column as an inline style.
All grid layout must now be implemented by consumers of the Foundation package. The now removed method of applying layout can be implemented by consumers by extending the base classes and adding the inline styling back in, OR any combination of CSS grid, SubGrid, and FAST's CSS bindings can be used to achieve the same results as before or more modern, flexible layouts depending on their needs.

Layout with CSS SubGrid

// data-grid.styles.ts
export const DataGridStyles = css`
  :host {
    display: grid;
    grid-auto-flow: row;
    grid-template-columns: repeat(
      ${x => x.columnDefinitions?.length ?? 1}, /* Use the grid's column definitions with a CSS binding */
      1fr
    );
  }
`;

// data-grid-row.styles.ts
export const DataGridRowStyles = css`
  :host {
    display: grid;
    grid-template-columns: subgrid;
    grid-column: 1 / span all;
    grid-auto-flow: row;
  }
`;

// data-grid-cell.styles.ts
export const DataGridCellStyles = css`
  :host {
    grid-column: ${x => x.gridColumn ?? 0}; /* Need to use a CSS binding here because cell positions can be remapped through the grid's column definitions. */
  }
`;

🎫 Issues

👩‍💻 Reviewer Notes

I updated DataGrid's stories with the above layout examples and added a new story that behaves more like a table where the grid overflows horizontally with a scrollbar when it runs out of space rather than the column widths shrinking and overflowing individually.

📑 Test Plan

Removed tests related to the inline styling. All other tests continue to pass.

✅ Checklist

General

[x] I have included a change request file using $ yarn change
[ ] I have added tests for my changes.
[x] I have tested my changes.
[x] I have updated the project documentation to reflect my changes.
[x] I have read the CONTRIBUTING documentation and followed the standards for this project.

Component-specific

[ ] I have added a new component
[x] I have modified an existing component
[ ] I have updated the definition file
[x] I have updated the configuration file

⏭ Next Steps

KingOfTac commented 8 months ago

@awentzel @bheston @scomea I ended up moving forward with completely removing the inline styling capabilities. PR title, description, and change files have all been updated.

scomea commented 8 months ago