icflorescu / mantine-datatable

The table component for your Mantine data-rich applications, supporting asynchronous data loading, column sorting, custom cell data rendering, context menus, nesting, Gmail-style batch row selection, dark theme, and more.
https://icflorescu.github.io/mantine-datatable/
MIT License
930 stars 67 forks source link

Contributing #425

Closed dlgoodchild closed 11 months ago

dlgoodchild commented 1 year ago

I thought I'd take a look at the codebase some and see if I can contribute to the move to v7. I think some documentation around contributing would go a long way to help encourage contributing. I've just been trying and it just feels like guess work.

Have you tried the following, with no global npm installs:

git clone https://github.com/icflorescu/mantine-datatable.git
cd mantine-datatable
npm install

This will fail due to peer dependencies with RTL. So try again with --force. Now you get a sea of warnings, which isn't overly confidence building that you're on the right track.

In any case, you've got dependencies installed. So the natural next step is npm run build to see what you get... What you get is instant fail:

root task build (turbo run build --no-daemon) looks like it invokes turbo and might cause a loop

No tasks were executed as part of this run.

 Tasks:    0 successful, 0 total
Cached:    0 cached, 0 total
  Time:    111ms

 ERROR  run failed: command  exited (1)

OK. So I'll try package/ by itself then. So:

cd package
npm install

Again, peer dependencies, so back to --force. Sea of warnings once more, but at least the dependencies are installed. Try npm run build. Starts off great, some green, then it errors a lot.

So without any documentation or guidance, contributing is not so clear. I'd really like to contribute, it's a great lib, but as it is currently, it just doesn't feel possible.

otobot1 commented 1 year ago

It's been a couple of years since I contributed, but I recall having weird errors when I tried setting the repo up locally. Have you tried using yarn instead of npm? It's what the repo is setup for and I remember when I realized that and started using it, it fixed at least a few of the problems I was having.

dlgoodchild commented 1 year ago

Just tested it, yarn seems to do the trick.

icflorescu commented 1 year ago

I'm still up to my ears into rewriting Mantine ContextMenu V7 & its docs website (hope to finish it later tonight, you can see my progress in the next branch here) and much of the work that I'm putting in there will be reused in Mantine DataTable here.

@dlgoodchild please don't be mad, but using yarn is not guesswork... but rather the first thing to do when dealing with a repo that includes a yarn.lock file and not a package-lock.json.

I'm sorry I haven't thought to mention it in the contributing guide, but I thought it was obvious. My bad.

I got the idea, though, and I'll make sure to update the guide as soon as possible.

One more thing to keep in mind - please make sure to stay on the next branch.

dlgoodchild commented 1 year ago

I wouldn't at all be mad, not at all. What's obvious to one person isn't necessarily so for the next person. I've never used yarn, never even so much as installed it. I'm aware of it being compatible with npm's package.json, but that's the height of it. It would lead one to believe it should be compatible, but I've since learned, tonight, that this is not the case.

This though, is why I speak of guesswork. Making assumptions that everyone uses the same toolset means someone who is unfamiliar with a particular toolset, may not even know said toolset is required.

The more information that can be provided around contributing the higher the chances are that people will actually do it. Lower barriers to entry help in every situation. I will add notes to the contributing page as I understand more about the project.

So, supposing I fork this and I start pushing to my next branch, what will be your acceptance criteria of the PR opened against next? Is it worth having a v7 branch so we can make partial PRs so that everyone can see the advancement being made in iterations?

icflorescu commented 1 year ago

Have a look at this next branch of Mantine ContextMenu, I'm currently on it and plan to reuse much of the code for Mantine DataTable tomorrow (it's 12:38am for me, so I'm gonna call it a day soon 😩) As you'll see in that branch, I dropped the monorepo idea, as I discovered a fairly simple way to collocate the package & docs website in a single project.

Also, please have a look at this comment in the other thread, there's some insight in there on why I switched from using esbuild to plain tsc and postcss in the build process... There's a lot more to the entire process than just porting createStyles function calls to pseudo-native css.

I've also discovered a few other breaking changes in a couple of Mantine components, but finding & fixing is not a problem with proper TS/linting.

One of the reasons I believe the docs website and code examples have to be rewritten is that I want to make sure everything works without breaking RSC, and - if possible - without forcing the users to type the use client directive when they include a <DataTable> in a RSC page.

To keep things simple and still have documentation websites for both versions without having to write a complicated GitHub pages deployment workflow relying on multiple branches, I'll replicate the repo the same way I did with Mantine ContextMenu: I'll copy the existing code into icflorescu/mantine-datatable-v6, and do the actual migration in icflorescu/mantine-datatable.

To understand what I mean: icflorescu.github.io/mantine-contextmenu icflorescu.github.io/mantine-contextmenu-v6

So basically we'll be able to do the migration in the next branch of this repo.

I'll think of more (and maybe be able to express myself clearer tomorrow).

Thanks again for your input and have a good night!

icflorescu commented 1 year ago

I do see the value in your idea of creating a stronger community around the project.

I lost a dear family member a few weeks ago, I am not completely myself yet, so I was unable to focus on work, and this issue was unanswered for 3 weeks, though the answer was rather obvious.

Thanks, @otobot1 for noticing it and taking the time to reply! 🙏

On the other hand, this happens with bigger open-source projects that even have financing and a strong company behind. I have a pending PR to Algolia's docsearch since 23rd of June, and nobody found a chance to accept it or reject it.

But that's understandable; when it comes to OSS, people will commit their time with passion yet sporadically, because they have lives to live and perhaps more pressing matters to attend to. Myself included. I have quite a few OSS projects, yet my GitHub sponsors account says "monthly estimated income: $10" 😐

icflorescu commented 11 months ago

Just wanted to let you know that I have released the first alpha of V7, and I've clarified a bit the contributing guide, see here and here.

Mantine DataTable V7

The amount of effort spent to rewrite the entire documentation website with Next.js 13/14 & RSC was gruesome, and could really use some help with finding potential bugs and things that don't work or look well in the examples.

Thanks again for your patience and support!