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

State/Prop fix, New features, Bug Fixes, Plug-ins #1065

Closed
wants to merge 1 commit into from
Closed

State/Prop fix, New features, Bug Fixes, Plug-ins #1065

wants to merge 1 commit into from

Conversation

patorjk
Copy link
Collaborator

@patorjk patorjk commented Nov 15, 2019

This PR is similar to #795 in that it's more for discussion, though technically a merge may work but smaller PRs might be better based on discussion.

Months ago I forked this repo. I'm using this table for a rather big project and I needed the flexibility to iterate quickly and add features that I needed. I'm moving that project to my internal codebase so I'll be discontinuing work on the public repo (which was only for my team). However, I added a number of improvements that I wouldn't mind giving back to this project since it's helped me tremendously. I'll list below the major changes.

  • packson.json and package-lock.json have been updated to use the latest packages. All vuneratabilies are gone, and all of the relevant test, rollup, etc, scripts have been updated. This was the first thing I did months ago and I've had no issues. This has also allowed me to use the latest version of react, material-ui, and important features like Hooks. (PR: Package.json overhaul #1077)
  • I changed the way State/Props work which I think would eliminate the need for the version 4 refactor. Issue TableState lost during re-render. #807 is a good example of this. When the table re-renders, all internal state is lost. This is a development nightmare and causes people manage the state themselves. In my version, the table doesn't reset state that isn't overriden in the "options" or "columns" parameters. So if you're only controlling rowsSelected, you won't lose your sort direction when the table re-renders. This seems like the most logical way it should work and follows the KISS principle. Making devs manage all of the state is just too much work and leads to lots of problems. Doing it this way eliminates the need for drastic API changes and re-writes. (PR: Updated table to make state persistent between renders #1086)
  • Using serverside filters does not require a separate API (ie, serverSideFilterList). This was originally why I forked the project (Using onBlur to activate the filter instead of using onFilterChange #857). My implementation uses the same API for both client and server side filtering, and simply allows for some customization when server side is set to true (through filterPopoverOptions). It is much easier to use, and does not cause the problems that a separate API does (ex: CustomFilterListRender not working when ServerSide = true #983).
  • A plug-in system has been setup. There are common renderers that users needs (ex: debounceSearchRender), and having the project include these types of plug-ins would make it friendlier to developers. (PR: Debounced Search Plug-in #1089)
  • A customBodyRenderLite method was developed that has better performance than customBodyRender. It is useful for tables with lots of data and columns (ie, 50k+ rows, 15+ columns, etc etc).
  • Sorting was split into an option called sortOrder. Having the sortDirection on the column object is confusing and causes problems (ex: sortDirection cannot be applied for more than one column. #966). It is also is a huge pain in the butt for people with server side tables. I think serverSide use is a blind spot for this project. The hand full of things I did in this area greatly eliminated pain points. Also, having a sortOrder option also opens the door for multi-column sorting down the road.
  • The responsive option was split into "displayMode" and "tableBodyHeight ". This allows one to set the height of the table independent of the display mode (ie: scroll, stacked, responsiveScroll). Having the table height tied to the responsive option is not flexible for all use cases. Also, it's bad for cases where you have a table in the sidebar and a table in the main display area.
  • This bug is fixed: The selection box is misplaced when I scroll the table (only safari) #900 - this is a bad bug and makes the table unusable in Safari and mobile Chrome if you have checkboxes. (PR: Fix 900 and refactored fixedHeaderOptions into fixedHeader and fixedSelectColumn #1088)
  • An expand all header option was added (Expand all rows Expandable Row Header? #740) (PR: Added expandable row header (expandableRowsHeader option) #1091)
  • checkboxColor option was added. Last time I checked this table still had issues with checkbox colors (ex: the filter checkboxes had the wrong highlight color). Also, allowing custom colors is nice.
  • readme.md was alphabetized - The current state of things makes finding API methods hard.
  • Various function signatures were changed to be consistent.

There may be more, but that's what I can remember off the top of my head.

This PR isn't meant as a slight and is simply an offer to better this project. Before chosing mui-datatables back in June, I did a head-to-head comparsion between it and material-table (which material-ui seems to be pushing and which has moved ahead of this table in popularity) and this table performed much better than material-table in all of my tests.

Edit:

Here's the link to the repo: https://github.com/patorjk/mui-dt

Looks like the PR only included the package-lock.json.

@patorjk
Copy link
Collaborator Author

patorjk commented Nov 15, 2019

Couple of other things just came to mind:

  • Handles dark mode for selection and hover colors.
  • Performance improvement: doesn't render extra divs when in scroll mode.
  • Alphabetized parameters in main file to make easier to track.

@gabrielliwerant
Copy link
Collaborator

Hey @patorjk, agreed with many of the suggestions and improvements here, and would be happy to have a look at them! I think opening each as a separate PR would be best so we could discuss and review each. Thanks so much for your support.

@patorjk
Copy link
Collaborator Author

patorjk commented Nov 29, 2019

I've begun putting in PRs, I'll update each bullet point above with the PR when I get it in. These are actually taking more time to do than I thought. Most of the PRs I'm planning are in that mui-dt repo. The two most important ones are the package.json updates and the state persistence update. However, after seeing another PR you have lined up, I really think the displayMode/tableBodyHeight idea should be considered. I'll try and work on a PR for that later this weekend.

@patorjk
Copy link
Collaborator Author

patorjk commented Nov 29, 2019

Also, I realized what went wrong with this PR. I didn't want to delete my mui-datatables repo since it had my branches in it, so I created a new repo and imported this repo into it for the mui-dt project. When creating this PR I used my mui-datatables fork instead of mui-dt - though it looks like I'm unable to create merge using the mui-dt repo since it's not considered a fork in github's eyes.

@patorjk patorjk closed this May 31, 2020
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.

The selection box is misplaced when I scroll the table (only safari)
2 participants