Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: Remove inline styling logic from DataGrid #6886

Closed

Conversation

KingOfTac
Copy link
Collaborator

@KingOfTac KingOfTac commented Jan 8, 2024

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

  1. Data Grid's grid-template-columns attribute is removed.
  2. Data Grid Row's grid-template-columns attribute is removed.
  3. Data Grid no longer generates the CSS string for grid-template-columns.
  4. Data Grid Row no longer applies the grid-template-columns as an inline style.
  5. Data Grid Cell no longer applies grid-column as an inline style.
  6. 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

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

Component-specific

⏭ Next Steps

@KingOfTac KingOfTac requested a review from scomea January 8, 2024 21:25
@KingOfTac KingOfTac changed the title fix: change FASTDataGridRow.updateRowStyle from a private arrow function property to protected member feat: Remove inline styling logic from DataGrid Jan 9, 2024
@KingOfTac
Copy link
Collaborator Author

@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
Copy link
Collaborator

scomea commented Jan 10, 2024

@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.

I am ok with it, but this is now a "notable" breaking change that should be advertised as such.

@janechu
Copy link
Collaborator

janechu commented Jun 14, 2024

I'm going to close this as part of our work on #6951 without merging due to the fact that this is technically a breaking change which is something we want to avoid right now.

@janechu janechu closed this Jun 14, 2024
@janechu janechu deleted the users/kingoftac/foundation/update-grid-row-style-method branch June 14, 2024 17:10
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.

5 participants