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

Issue 376: Updated MUI component to used styled component #433

Merged

Conversation

bingeboy
Copy link
Contributor

Issue 376: Updated MUI component to used styled component

This pull requests adds support to the styled components found on the user's document page.

This PR:

Resolves #376


The files this PR effects:

Components

<DocumentTable />

Tests

Currrent there are zero tests for this component


Screenshots (if applicable):

image

@bingeboy
Copy link
Contributor Author

@xscottxbrownx @andycwilliams @leekahung is this all that was needed or are there additional components that need to be replaced?

@xscottxbrownx
Copy link
Contributor

xscottxbrownx commented Sep 22, 2023

I think you have completely misinterpreted Issue #376

The goal of that issue is to get rid of the styled components in those two components/files, and use the MUI based styling (or sx prop in MUI if needed.)

We are trying to get to and enforce a consistent way of styling/css...
First, we got rid of the separate styles.css file.
Second, we are now trying to get rid of all the use of styled components everywhere and use the MUI built-in so the codebase is consistent.

@xscottxbrownx
Copy link
Contributor

Also I just realized, that Issue #376 is tied/linked to Issue #219 ...

They both import the same styles, so honestly it seems these components would need to be refactored at same time or else they will break.

The source of the styled components styling:
src/components/Table/TableStyles.js

Components importing/using the styling instead of MUI:
src/components/Contacts/ContactListTable.jsx
src/components/Contacts/ContactListTableRow.jsx
src/components/Documents/DocumentTable.jsx
src/components/Documents/DocumentTableRow.jsx


Not sure if it would be more efficient to make new components, change the theme.js to use these styles by default, or what honestly. I haven't researched the way I would go about making these changes yet.

@leekahung
Copy link
Contributor

Think we could switch this up with MUI data grid https://mui.com/x/react-data-grid/

@bingeboy
Copy link
Contributor Author

There are a few ways to support custom styles with MUI. Seemed like there were existing overrides in the theme.js file so I was removing the custom components in favor of keeping the existing MUI components based on feedback.

There was another PR #408 I was working on which requested the removal of classes being declared in the component file. Alternatively we could use hooks but that seemed like it would add complexity in this case. I haven't been brief on architecture decisions for MUI styling so I'm following the MUI docs.

@bingeboy bingeboy added the question Further information is requested label Sep 25, 2023
@bingeboy
Copy link
Contributor Author

I believe I removed all the uses of the <StyledTableCell /> and <StyledTableRow /> . I can upgrade the table to a <DataGrid /> if that is within the scope of this ticket.

@bingeboy
Copy link
Contributor Author

bingeboy commented Oct 3, 2023

@leekahung so I have the <DataGrid /> mostly working on my local on another branch. I'd recommend making that another ticket though.

@leekahung
Copy link
Contributor

@leekahung so I have the <DataGrid /> mostly working on my local on another branch. I'd recommend making that another ticket though.

Sounds good

Copy link
Contributor

@timbot1789 timbot1789 left a comment

Choose a reason for hiding this comment

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

This all looks good to me. I like the idea of keeping things as close to the MUI docs as possible. We don't really need super sophisticated UI designs for this, beyond getting the point across.

@bingeboy
Copy link
Contributor Author

bingeboy commented Oct 4, 2023

Squashing thx

@bingeboy bingeboy force-pushed the issue-376-transition-from-styled-components-to-mui branch from f1c12b9 to 5b447b9 Compare October 4, 2023 18:03
@bingeboy bingeboy merged commit 9fcb38f into Development Oct 4, 2023
@bingeboy bingeboy deleted the issue-376-transition-from-styled-components-to-mui branch October 4, 2023 18:09
@xscottxbrownx
Copy link
Contributor

@milofultz

Guess I had chances to look at this before merge and too late now,

but for future reference you don't need to pass the theme provider to each individual component. It's already passed in App.jsx to all children (meaning every component of the app.)


And I'm not a fan of making the variables for theme.pallete.primary.main and .slight. I'd prefer we do this to all the colors or none. Just inconsistent.
As dev, do I now need to do theme.pallete.primary.light or themePalletePrimaryLight?
If theme.pallete.primary.light was set to themePalletePrimaryLight then either would work, but you see the point in the inconsistency...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
question Further information is requested
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants