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

Fix ResponsiveDataTable bug #846

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

mdkaifansari04
Copy link
Contributor

Notes for Reviewers
This PR fixes the bug if ResponsiveDataTable it has a required prop as columns and option props as tableCols( both the prop type is same) which is displayed in MUI datatable, if someone doesn't pass tableCols the table breaks.

So I fixed this by passing the both columns or tableCols, if the optional prop is not passed it wont break anymore. Altough the any big props changes may lead to make the chages all over where it is been used.

This PR fixes #635

Signed commits

  • Yes, I signed my commits.

@capricorn-32
Copy link
Contributor

The MUIDatatable documentation specifies that there is no tableCols prop. However, in the codebase, there are inconsistencies: some components pass both columns and tableCols with same arguments, while others use only columns.

To eliminate this ambiguity, it’s important to note that columns is designed to define the table headers and manage the visibility of specific columns. Standardizing the use of columns across the codebase would avoid confusion.

@mdkaifansari04
Copy link
Contributor Author

mdkaifansari04 commented Dec 12, 2024

You are completely right @capricorn-32 but sistent component are the base component which is been used across all the projects of Layer5, making any major change in the prop type or removing any prop, will lead to a major break and leads to breaking change across all over the places it is been used.

Although all the components is been documented.

Just to avoid all this pain, I tried to make a change in a way a that, it should be fixed without changing the behaviour or the current component.

I can remove the tableCols props as there is no use of it, but it will lead us to major change in all over the code bases.

@capricorn-32
Copy link
Contributor

@mdkaifansari04 I am pointing this because in future it will more pain to work around this. Lets see, what decision should be made to prevent this.

@mdkaifansari04
Copy link
Contributor Author

One way to fix this would be change in component and update the docs,

After just the release we have to make changes in the corresponding projects as I've access to all the projects eventually.

I can make the changes but I'm busy in some school stuff might not be possible now.

If anyone shows up we can create issues for each repo.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

ResponsiveDataTable bug and readme updates
2 participants