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

Styling and custom component for DataGrid panels #3033

Merged
merged 4 commits into from
Feb 27, 2025

Conversation

jamesricky
Copy link
Contributor

@jamesricky jamesricky commented Jan 3, 2025

Description

This implements theme-styling of the filter and column panels to more closely match the comet design.
No changes in the application are needed for this to take effect.

This also implements a custom DataGridPanel component which improves on the styling and enables the full-screen/mobile variants of the panels defined by the comet design.

Due to @comet/admin-theme not having a dependency on @comet/admin the new component must be added to the applications theme manually. This will be changed in v8, as the @comet/admin-theme package is being merged into the @comet/admin package with #3479

Example theme: src/theme.ts:

import { DataGridPanel } from "@comet/admin";
import { createCometTheme } from "@comet/admin-theme";
import type {} from "@mui/x-data-grid/themeAugmentation";

export const theme = createCometTheme({
    components: {
        MuiDataGrid: {
            defaultProps: {
                components: {
                    Panel: DataGridPanel,
                },
            },
        },
    },
});

Acceptance criteria

Screenshots

Custom DataGridPanel (preferred - needs to be added in the theme)

Columns Panel Filters Panel
Custom ColumnsPanel Desktop Custom FilterPanel Desktop
Custom ColumnsPanel Mobile Custom FilterPanel Mobile

MUIs default GridPanel (works out of the box)

Columns Panel Filters Panel
Default ColumnsPanel Desktop Default FilterPanel Desktop
Default ColumnsPanel Mobile Default FilterPanel Mobile

Changed to be made in separate tasks/PRs:

  • Move theme values added to Demo-Admin to createCometTheme() – this may require merging the @comet/admin-theme into the @comet/admin package COM-1649
  • Fix styling of value input when using the "Is any of" operator COM-1650
  • Implement the new columns-panel according to the design COM-1651

Further information

@jamesricky jamesricky force-pushed the full-screen-grid-panels branch from c845d36 to 376ff70 Compare January 3, 2025 15:31
@jamesricky jamesricky force-pushed the full-screen-grid-panels branch from 376ff70 to bd69a80 Compare January 27, 2025 07:40
@vivid-planet vivid-planet deleted a comment from sonarqubecloud bot Jan 27, 2025
@jamesricky jamesricky force-pushed the full-screen-grid-panels branch from bd69a80 to 72065da Compare January 27, 2025 10:28
@jamesricky jamesricky force-pushed the full-screen-grid-panels branch 2 times, most recently from adf5ad9 to 3500fcb Compare February 19, 2025 13:43
@jamesricky jamesricky self-assigned this Feb 19, 2025
@jamesricky jamesricky force-pushed the full-screen-grid-panels branch 5 times, most recently from dbc52be to 6391cd7 Compare February 20, 2025 16:11
@jamesricky jamesricky force-pushed the full-screen-grid-panels branch from 6391cd7 to acc4df0 Compare February 20, 2025 16:29
@jamesricky jamesricky marked this pull request as ready for review February 20, 2025 16:29
Copy link
Collaborator

@johnnyomair johnnyomair left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems like you're doing a lot of workarounds just to make it backwards compatible. Would it be easier if this PR would target next?

@jamesricky
Copy link
Contributor Author

It seems like you're doing a lot of workarounds just to make it backwards compatible. Would it be easier if this PR would target next?

I wouldn't really call it workarounds, it's basically just two different things:

  • Styling the existing panels from MUI in the Theme
  • Adding a custom component for the thing's don't exist or can't be styled in MUIs implementation

I suppose we could remove part of the theme-styling in next, or move it form the theme into the custom panel component since the custom panel will become the default in the theme. But I don't think there's that much code to be removed then, mostly just moved into another file.

Or is there something specific you mean?

@johnnyomair
Copy link
Collaborator

I wouldn't really call it workarounds, it's basically just two different things:

You're right, they aren't really workarounds, sorry.

Or is there something specific you mean?

I was mostly thinking about this:

<IntlProvider locale="en" messages={getMessages()}>
  <IntlContext.Consumer>
    {(intl) => (
      <MuiThemeProvider theme={getTheme(intl)}>{/* ... */}</MuiThemeProvider>
    )}
  </IntlContext.Consumer>
</IntlProvider>

It's odd that we need to do this in every application. In next (with the theme package merged) we could call useIntl() in the MuiThemeProvider and merge the passed theme with the locale messages, resulting in no changes (and no new context consumer!) in App.tsx.

@johnnyomair
Copy link
Collaborator

You're right, they aren't really workarounds, sorry.

Actually, having to define the messages in the theme in the application code seems like a workaround to me 😁

@jamesricky
Copy link
Contributor Author

It's odd that we need to do this in every application. In next (with the theme package merged) we could call useIntl() in the MuiThemeProvider and merge the passed theme with the locale messages, resulting in no changes (and no new context consumer!) in App.tsx.

In next we'll add this to the theme by default so there won't be the need to add it in the project: https://vivid-planet.atlassian.net/browse/COM-1649

In main this is optional, it can be added to the theme of projects that require it, the default MUI component will be used otherwise.

@jamesricky jamesricky force-pushed the full-screen-grid-panels branch from 905e315 to 3d7f9a8 Compare February 26, 2025 17:16
@jamesricky
Copy link
Contributor Author

@johnnyomair as discussed, I removed the custom localeText values that required intl.

@johnnyomair johnnyomair merged commit 7d8c36e into main Feb 27, 2025
11 checks passed
@johnnyomair johnnyomair deleted the full-screen-grid-panels branch February 27, 2025 08:44
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.

2 participants