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

styled MUI data grid #17

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

styled MUI data grid #17

wants to merge 5 commits into from

Conversation

binhttran
Copy link
Collaborator

styled MUI data grid

@binhttran binhttran changed the title Lexicon table styled MUI data grid Oct 25, 2024
Copy link
Collaborator

@hrprice hrprice left a comment

Choose a reason for hiding this comment

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

Great start! Just a few comments.

.DS_Store Outdated
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is a mac specific file that doesn't need to be in the repo

import { Box, Stack, Button } from '@mui/material';
import { alpha, createTheme, styled } from '@mui/material/styles';

const theme = createTheme({
Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's hold off on theme colors for now as it will be something we implement globally once we have a color palette from Ruby.

@@ -0,0 +1,118 @@
import { GetAllLexEntriesQuery, useGetAllLexEntriesQuery } from '../graphql/lexicon/lexicon.ts';
import { DataGrid, GridColDef,DataGridProps } from '@mui/x-data-grid';
import React, { useState } from 'react';
Copy link
Collaborator

Choose a reason for hiding this comment

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

React is an unused import

headerName: 'Primary',
width: 200,
headerAlign: 'center',
headerClassName: 'boldHeader'
Copy link
Collaborator

Choose a reason for hiding this comment

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

This classname is never defined so I don't think this does anything

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

oh yeah I was trying to make the headers bold, but it is not working so I will remove it, definitely not a priority right now

];


const LexiconTable = () => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

You are on the right track here but I think it will be much easier to adapt the crud datagrid example from the MUI docs to our needs, rather than trying to code our own solution from scratch. I would recommend starting with the example at https://mui.com/x/react-data-grid/editing/ and editing it where necessary to handle our data. I think it should be as simple as just changing the GridColDef and adding in the query for the lexicon data.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yea having the implementation follow the CRUD sample will make life easier. This is pretty close.

Just to add some pointed areas to have this implementation match the sample.

  1. Having the "Add Row" button as part of the DataGrid slot property like in the demo and removing the "Remove Row` button
  2. Having the "Action" column to handle things like having an edit and delete buttons.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Did some research and these are autogenerated by tsc and don't need to be pushed.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Same goes for this one

Copy link
Contributor

@cbolles cbolles left a comment

Choose a reason for hiding this comment

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

Looks really good. Really good start, I added a few suggestions myself and agree with Henry's points as well

}
})
const StyledDataGrid = styled(DataGrid)<DataGridProps>(({ theme }) => ({
'&.MuiDataGrid-root': {
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd recommend removing the custom CSS. We should be able to get the functionality we want with the Global theme when the time comes

</Button>
</Stack>

<div style={{display:'flex', flexDirection: 'column', width: '100%'}}>
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of using a div, I recommend using a Box or similar. Then the container will follow the theme provided settings at the global level.

];


const LexiconTable = () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Yea having the implementation follow the CRUD sample will make life easier. This is pretty close.

Just to add some pointed areas to have this implementation match the sample.

  1. Having the "Add Row" button as part of the DataGrid slot property like in the demo and removing the "Remove Row` button
  2. Having the "Action" column to handle things like having an edit and delete buttons.

@binhttran binhttran closed this Oct 30, 2024
@binhttran binhttran reopened this Oct 30, 2024
@binhttran
Copy link
Collaborator Author

see most recent changes in lexicon table branch

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.

3 participants