Implement #28 #35

otobot1 · 2022-09-16T10:25:15Z

Here is my first stab at implementing the functionality discussed in #28. Allows the consumer to pass in any custom component. That custom component is passed to the new DataTableRowParent wrappers around each DataTableRow and will be rendered beneath the row if isExpanded is set.
isExpanded is calculated in the DataTable itself.
The custom component is wrapped in a Collapse component from @mantine/core. The expansion animation behaviour can be customized via the expandedRow.collapseProps property.

Please let me know what you think. Should there be more features added? Fewer?

Also please note that this is untested and unlinted. The build and dev scripts wouldn't work for me - I suspect that may be because I'm on Windows, but I haven't had much of a chance yet to troubleshoot.

icflorescu · 2022-09-16T20:10:50Z

Thank you for the PR! I'll have an in-depth look at it over the next few days.

Meanwhile, just from a quick view, I have a few comments:
I'm on Windows too, but I'm using WSL2, you should give it a try :-)
Please try to lint and enable prettier (so it will respect .prettierrc in the repo root); otherwise it's going to be quite difficult for me to track what really changed.

Trying to run the code (again, without really reviewing it) shows the following console warning:
next-dev.js?878b:20 Warning: validateDOMNesting(...): <div> cannot appear as a child of <tbody>
I guess it's obvious why this is happening.

Right now, if I'm adding this on the complex usage example:

// ...
expandedRow={{
  item: ({ firstName }) => <Box sx={{ border: '1px solid red' }}>{firstName}</Box>,
}}
// ...

...here's is what I get:

I think the idea of using a Collapse is good, but it should probably be put inside a <tr><td colSpan={x}>{/* collapse here */}</td></tr>, or something like that, in order to obtain a valid DOM and ensure that it spans the entire width of the table. Or maybe - with absolute positioning - inside a zero-width column at the end of the row. Both approaches would be kind of hacky, though...

There are some other things that should be taken into account, i.e.:

what happens if the table is horizontally scrollable?
once the user "expands" a row, he should be able to "collapse" it back (in other words it should work like a toggle)

Please allow me a few days to have a better look at it.

otobot1 · 2022-09-16T20:56:13Z

Thank you for the PR! I'll have an in-depth look at it over the next few days.

Meanwhile, just from a quick view, I have a few comments: I'm on Windows too, but I'm using WSL2, you should give it a try :-) Please try to lint and enable prettier (so it will respect .prettierrc in the repo root); otherwise it's going to be quite difficult for me to track what really changed.

If I install the Prettier and ESLint VSCode extensions will that work? Or are there more steps I need to take? I've never worked with a linter or prettier before. I'll give WSL2 a shot as well!

I think the idea of using a Collapse is good, but it should probably be put inside a <tr><td colSpan={x}>{/* collapse here */}</td></tr>, or something like that, in order to obtain a valid DOM and ensure that it spans the entire width of the table. Or maybe - with absolute positioning - inside a zero-width column at the end of the row. Both approaches would be kind of hacky, though...

Ah I see. I think at first glance I like the first solution more, but I'll play around with it and see if any other ideas come to mind.

There are some other things that should be taken into account, i.e.:

what happens if the table is horizontally scrollable?

I think it's reasonable if the Collapse is always as wide as the entire row, so in this case then it would scroll along with the rest of the table. I think any alternative would be pretty hacky and not really worth it.

once the user "expands" a row, he should be able to "collapse" it back (in other words it should work like a toggle)

That was my intention, but I guess I forgot to add that in. I'll try and get to that today.

Please allow me a few days to have a better look at it.

Sounds good. I'll see if I can get it to run lol.

otobot1 · 2022-09-16T22:41:50Z

Any idea why running the dev script doesn't release the ports? I'm doing it from a WSL2 terminal now. I'm having to reload the dev server constantly as I can't seem to get hot-swapping to work.

width works correctly now toggles correctly on click red border still unsolved

otobot1 · 2022-09-17T03:17:59Z

I fixed the mentioned issues. I wasn't able to figure out how to get rid of the red border around the expandedRow though... I'm still learning how to debug classes created by createStyles.

otobot1 · 2022-09-18T00:11:57Z

Didn't mean to mark this as Ready for review. The red border still needs to be fixed.

otobot1 · 2022-09-19T00:07:36Z

I glanced at the code again and realized that the red border was added in by the test code I copied from your comment. Whoops. I believe I've fixed all of the bugs, and this is ready for review.

icflorescu · 2022-09-19T17:44:53Z

Any idea why running the dev script doesn't release the ports? I'm doing it from a WSL2 terminal now. I'm having to reload the dev server constantly as I can't seem to get hot-swapping to work.

Absolutely no idea. It works fine on my setup...

icflorescu · 2022-09-19T17:46:48Z

I glanced at the code again and realized that the red border was added in by the test code I copied from your comment. Whoops. I believe I've fixed all of the bugs, and this is ready for review.

Sorry for not being able to reply sooner. Yes, I've added the red border in my comment so we can easily see what's boing rendered. It's a common quick & dirty practice :-)

icflorescu · 2022-09-19T17:47:40Z

Looking at it now. Thanks again for your effort!

icflorescu

Might have to refactor a few things after merging to keep coding style consistent, I hope you won't mind.

I'll also test all the available examples to see how this affects the already existent features. One thing I've discovered so far: all rows have cursor: pointer now, and they shouldn't. It's probably due to the fact that each DataTableRow unconditionally receives an onClick handler, and it always sets this class: [classes.withClickHandler]: onClick. I'll see what I cand do about it.

I'll also add an example to illustrate the new functionality.

icflorescu · 2022-09-19T17:50:04Z

package/DataTable.tsx

@@ -187,7 +188,7 @@ export default function DataTable<T>({
  };

  /**
-   * React hooks linting rule would reccomend to also include the `useDobouncedState` setters
+   * React hooks linting rule would recommend to also include the `useDobouncedState` setters


Nice catch ;-)

Y'know, I just noticed that it says useDobouncedState instead of useDebouncedState. Didn't catch that one ;-)

icflorescu · 2022-09-19T18:19:15Z

Please allow me a an hour or so to add an example and see if any refactoring is needed before publishing a new version on npm. I think we should probably bump it to v1.7.0, as this is a major new functionality.

icflorescu · 2022-09-19T19:32:47Z

Quick tip:

I was tempted to use !important in Mantine's CSS-in-JS before, but I found that it doesn't work as expected if you write it like this:

expandedRow: {
  padding: '0 !important'
}

So I'll change it to:

expandedRow: {
  '&&': {
    padding: 0
  }
}

See more info here: https://stackoverflow.com/questions/62660480/is-there-a-way-to-increase-specificity-by-adding-the-element-with-emotion

icflorescu · 2022-09-19T20:58:34Z

A couple of regressions I'm noticing now, besides the cursor: pointer class being applied on each row:

This doesn't work well with striped; it remains to be seen whether we'll be able to fix it or just add a notice about it;
An additional border is now visible under the last row of the DataTable (especially annoying when using <DataTable withBorder />) - I think I'll be able to fix this one

otobot1 · 2022-09-19T21:23:18Z

A couple of regressions I'm noticing now, besides the cursor: pointer class being applied on each row:

This doesn't work well with striped; it remains to be seen whether we'll be able to fix it or just add a notice about it;

An additional border is now visible under the last row of the DataTable (especially annoying when using <DataTable withBorder />) - I think I'll be able to fix this one

Can you share a picture of what happens when the table is striped?

otobot1 · 2022-09-19T21:26:37Z

Thanks for the tip! I knew !important wasn't a great option, but I'm not very familiar with the other syntax. I'll check out that SO link.

icflorescu · 2022-09-19T21:27:45Z

Also, I'm gonna have to refactor this:

  const { expandRowOn = 'click', expandMultiple = false, expandFirst, collapseProps } = expandedRow ?? {};

...because it triggers a re-render on every row click, regardless of whether you're providing an expandedRow option or not...

icflorescu · 2022-09-19T21:32:17Z

A couple of regressions I'm noticing now, besides the cursor: pointer class being applied on each row:

This doesn't work well with striped; it remains to be seen whether we'll be able to fix it or just add a notice about it;

An additional border is now visible under the last row of the DataTable (especially annoying when using <DataTable withBorder />) - I think I'll be able to fix this one

Can you share a picture of what happens when the table is striped?

All "normal" rows appear dark:

The "expanded" rows are light:

It happens due to the way striped rows are styled in the underlying Mantine table, with table[data-striped] tbody tr:nth-of-type(odd).

otobot1 · 2022-09-19T21:36:21Z

Also, I'm gonna have to refactor this:
  const { expandRowOn = 'click', expandMultiple = false, expandFirst, collapseProps } = expandedRow ?? {};
...because it triggers a re-render on every row click, regardless of whether you're providing an expandedRow option or not...

Ah that's not good... I'm still getting familiar with all of the things that can trigger unnecessary re-renders. Thanks for taking the time to go through my less-than-perfect code :D

otobot1 · 2022-09-19T21:41:16Z

All "normal" rows appear dark:

The "expanded" rows are light:

It happens due to the way striped rows are styled in the underlying Mantine table, with table[data-striped] tbody tr:nth-of-type(odd).

Ahh that makes sense. I wonder if, instead of having the expanded rows in their own tr, the main row and expanded row could be wrapped by a single tr and a <Stack>

icflorescu · 2022-09-19T21:42:07Z

All code is perfectible, none is perfect :-) Mine is always far from perfect :-).

Of course I'm taking the time; you were generous enough and willing to commit your own time to the project.

If you'll look through the comments in Mantine's issues & discussions, you'll see that many people are asking for features, but so few understand that open-source is not just about using other people's work, but also about contributing.

Thank you for putting time into this!

icflorescu · 2022-09-19T21:50:22Z

main row and expanded row could be wrapped by a single tr and a Stack

Wouldn't that result in invalid HTML?

icflorescu · 2022-09-19T21:59:59Z

I did a bit of refactoring (hope you won't mind); mostly changed DataTableRow to be a single function/component.
I published everything to main, didn't release to npm yet, and I've also created a next branch (I should have done this from the beginning for adding new features, my mistake).
I've added a row expansion example with a "work in progress notice".

So, let's continue working on the next branch. Pull it, and if you'll run it locally you can test the feature at http://localhost:3000/examples/expanding-rows

Example page text is in /docs/pages/examples/expanding-rows.tsx, example code in /docs/examples/ExpandingRowsExample.tsx.

otobot1 · 2022-09-19T22:13:56Z

main row and expanded row could be wrapped by a single tr and a Stack

Wouldn't that result in invalid HTML?

🤷 I have no idea. I only learned how HTML tables worked last week 😆 Almost all of my prior experience has been pure JS/TS.

I did a bit of refactoring (hope you won't mind); mostly changed DataTableRow to be a single function/component. I published everything to main, didn't release to npm yet, and I've also created a next branch (I should have done this from the beginning for adding new features, my mistake). I've added a row expansion example with a "work in progress notice".

I don't mind at all 😆 My implementation was a little bit clunky for sure.

So, let's continue working on the next branch. Pull it, and if you'll run it locally you can test the feature at http://localhost:3000/examples/expanding-rows

Example page text is in /docs/pages/examples/expanding-rows.tsx, example code in /docs/examples/ExpandingRowsExample.tsx.

Sounds good. It might be a day or two before I have time though.

icflorescu · 2022-09-21T14:50:53Z

It might be a day or two before I have time though.

No problem, I'm a bit up-to-my ears in other stuff too.

It will have to wait a few days anyway, because I still couldn't understand where the unnecessary re-renders are coming from.
Considering that people are already using the library in production, I'm reluctant to push this to npm until I can solve it.
I'll have to rebuild the next branch, because I've pushed some updates to master and the branches are now quite out of sync and difficult to reconcile.
Will let you know when ready.

otobot1 · 2022-09-23T23:50:30Z

No problem, I'm a bit up-to-my ears in other stuff too.

It will have to wait a few days anyway, because I still couldn't understand where the unnecessary re-renders are coming from. Considering that people are already using the library in production, I'm reluctant to push this to npm until I can solve it. I'll have to rebuild the next branch, because I've pushed some updates to master and the branches are now quite out of sync and difficult to reconcile. Will let you know when ready.

I see there is a row-expansion branch now. I'm assuming that's where this lives at the moment? I feel like poking at #40 first, but I might have a chance to work on some of the outstanding issues for this feature over the weekend. Is this the best place to keep discussing this?

otobot1 · 2022-09-23T23:54:09Z

Oh is row-expansion for #43 ? And this feature is still being developed on next?

otobot1 added 3 commits September 16, 2022 00:19

enable passing in of custom component

deb44ed

implement additional functionality

edd66c3

fix type declaration

3d6d0b2

otobot1 added 3 commits September 16, 2022 16:54

linted and formatted

ecac54a

only call useStyles once

2c52401

fixed most initial issues

36b6369

width works correctly now toggles correctly on click red border still unsolved

otobot1 marked this pull request as ready for review September 18, 2022 00:10

otobot1 marked this pull request as draft September 18, 2022 00:11

otobot1 marked this pull request as ready for review September 19, 2022 00:06

icflorescu self-assigned this Sep 19, 2022

icflorescu added the enhancement New feature or request label Sep 19, 2022

icflorescu approved these changes Sep 19, 2022

View reviewed changes

icflorescu merged commit 9733f27 into icflorescu:main Sep 19, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement #28 #35

Implement #28 #35

otobot1 commented Sep 16, 2022 •

edited

icflorescu commented Sep 16, 2022

otobot1 commented Sep 16, 2022 •

edited

otobot1 commented Sep 16, 2022 •

edited

otobot1 commented Sep 17, 2022

otobot1 commented Sep 18, 2022

otobot1 commented Sep 19, 2022

icflorescu commented Sep 19, 2022

icflorescu commented Sep 19, 2022

icflorescu commented Sep 19, 2022

icflorescu left a comment

icflorescu Sep 19, 2022

otobot1 Sep 19, 2022

icflorescu commented Sep 19, 2022

icflorescu commented Sep 19, 2022 •

edited

icflorescu commented Sep 19, 2022

otobot1 commented Sep 19, 2022

otobot1 commented Sep 19, 2022

icflorescu commented Sep 19, 2022

icflorescu commented Sep 19, 2022

otobot1 commented Sep 19, 2022

otobot1 commented Sep 19, 2022 •

edited

icflorescu commented Sep 19, 2022

icflorescu commented Sep 19, 2022 •

edited

icflorescu commented Sep 19, 2022

otobot1 commented Sep 19, 2022 •

edited

icflorescu commented Sep 21, 2022

otobot1 commented Sep 23, 2022

otobot1 commented Sep 23, 2022

Implement #28 #35

Implement #28 #35

Conversation

otobot1 commented Sep 16, 2022 • edited

icflorescu commented Sep 16, 2022

otobot1 commented Sep 16, 2022 • edited

otobot1 commented Sep 16, 2022 • edited

otobot1 commented Sep 17, 2022

otobot1 commented Sep 18, 2022

otobot1 commented Sep 19, 2022

icflorescu commented Sep 19, 2022

icflorescu commented Sep 19, 2022

icflorescu commented Sep 19, 2022

icflorescu left a comment

Choose a reason for hiding this comment

icflorescu Sep 19, 2022

Choose a reason for hiding this comment

otobot1 Sep 19, 2022

Choose a reason for hiding this comment

icflorescu commented Sep 19, 2022

icflorescu commented Sep 19, 2022 • edited

icflorescu commented Sep 19, 2022

otobot1 commented Sep 19, 2022

otobot1 commented Sep 19, 2022

icflorescu commented Sep 19, 2022

icflorescu commented Sep 19, 2022

otobot1 commented Sep 19, 2022

otobot1 commented Sep 19, 2022 • edited

icflorescu commented Sep 19, 2022

icflorescu commented Sep 19, 2022 • edited

icflorescu commented Sep 19, 2022

otobot1 commented Sep 19, 2022 • edited

icflorescu commented Sep 21, 2022

otobot1 commented Sep 23, 2022

otobot1 commented Sep 23, 2022

otobot1 commented Sep 16, 2022 •

edited

otobot1 commented Sep 16, 2022 •

edited

otobot1 commented Sep 16, 2022 •

edited

icflorescu commented Sep 19, 2022 •

edited

otobot1 commented Sep 19, 2022 •

edited

icflorescu commented Sep 19, 2022 •

edited

otobot1 commented Sep 19, 2022 •

edited