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

feat: adds resizable columns #490

Merged
merged 15 commits into from
Dec 21, 2023
Merged

Conversation

gfazioli
Copy link
Contributor

@icflorescu In this PR, I added the ability to resize one or more columns.

Currently, I used the same provider and hook used for dragging and toggling. I'm not sure if this is okay in terms of style.
Of course, it would be possible to rename the context/provider, for example, to DataTableDragToggleResizeColumnsProvider or DataTableColumnsProvider.

The issue is with the useDragToggleColumns hook, as it is public and indicated in the documentation. For compatibility reasons, a new one should be created, such as useDataTableColumns, and the previous one should be deprecated (with a console warning).

Anyway, let me know what you think

image

className="mantine-datatable-header-resizable-knob"
style={{
right: rem(deltaX),
//backgroundColor: 'orange',
Copy link
Owner

Choose a reason for hiding this comment

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

I assume this was there for testing, right?
I'm thinking It may be a good idea, though, to provide some visual feedback to the user as to give a clue that a column is resizable.
Perhaps we could indeed slightly change its background color on hover, to a semi-transparent Mantine active color? I believe there's a CSS var for that, I'll have a look.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I assume this was there for testing, right?

yep, sorry, I forgot to delete it 😉
I was thinking the same thing too. Feel free to do some tests, and the idea of the hover might work 👍

Copy link
Owner

Choose a reason for hiding this comment

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

No need to be sorry. Seeing this actually triggered me to thnk about the visual feedback 😉

@icflorescu
Copy link
Owner

Outstanding work, thanks a lot!

I have another minor suggestion: maybe we could make the "knob" a bit wider? I think 2px is a bit too narrow and can be difficult to target with a less-precise mouse. Perhaps we could make it 4 or 5px and place it over the column border... I'll investigate this a bit further.

I completely agree with your suggestion to rename the useDragToggleColumns hook to useDataTableColumns while deprecating the old name (and remove it in a future version).

@gfazioli
Copy link
Contributor Author

Outstanding work, thanks a lot!

Thank you

I have another minor suggestion: maybe we could make the "knob" a bit wider? I think 2px is a bit too narrow and can be difficult to target with a less-precise mouse. Perhaps we could make it 4 or 5px and place it over the column border... I'll investigate this a bit further.

yes, I agree. In fact, during development, I had to manage the table with and without borders. In CSS, when the table has no borders, the 'knob' is actually thicker.

I completely agree with your suggestion to rename the useDragToggleColumns hook to useDataTableColumns while deprecating the old name (and remove it in a future version).

awesome 😉

@icflorescu icflorescu added documentation Improvements or additions to documentation enhancement New feature or request code quality Improve code readability labels Dec 21, 2023
@@ -105,6 +106,9 @@ export const DataTableHeader = forwardRef(function DataTableHeader<T>(
sortable={sortable}
draggable={draggable}
toggleable={toggleable}
// we won't display the resize handle for the last column
Copy link
Owner

Choose a reason for hiding this comment

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

Good thinking here!

@icflorescu
Copy link
Owner

This looks good enough for me now.

I've renamed the components as you suggested and deprecated the old useDragToggleColumns hook in favor of useDataTableColumns. I'm a bit reluctant to put deprecation warnings in the console, though. Is it really necessary, I wonder?

I also changed a bit the CSS to improve the visual feedback, but of course, I may be subjective.

Please let me know what you think.
If you agree with the changes, I'd like to publish the new release to npm :-)

@gfazioli
Copy link
Contributor Author

image

@gfazioli
Copy link
Contributor Author

If you agree with the changes, I'd like to publish the new release to npm :-)

sounds good to me 👍

@icflorescu icflorescu merged commit 6b9beea into icflorescu:next Dec 21, 2023
@gfazioli
Copy link
Contributor Author

I'm a bit reluctant to put deprecation warnings in the console, though. Is it really necessary, I wonder?

From one side, a rename is a breaking change, however, since this new feature is relatively young, maybe it is enough to rename the hook without creating a new one with the working.

@icflorescu
Copy link
Owner

I did create the new one named useDataTableColumns, but also kept the previously named useDragToggleColumns, like so:

/**
* @deprecated This hook is deprecated and will be removed in a future version. Please use the `useDataTableColumns` hook instead.
*/
export const useDragToggleColumns = useDataTableColumns;
// todo remove the above in a future version (maybe 7.4?)

This way there's no significant bundle size impact, and we'll remove it at some point later.
What I meant to say is that I was reluctant to add deprecation warnings in the console, but I did add a @deprecated js-docs tag, and AFAIK all code editors should be smart enough to highlight it.

@icflorescu
Copy link
Owner

Thanks a lot, again, for your effort!

Would you mind terribly if I'd send you a collaborator invitation to the repo at some point in the near future - in other words, add you as a co-maintainer?
You've probably noticed that the library gained a lot of traction lately, and sometimes I am unable to properly keep up with its maintenance.

All in all, it's a nice part-time job - perks include:

  • no financial compensation
  • lots of people asking for issues, very few people coming up with meaningful PRs
  • pressure when Mantine comes up with a new version

What do you think? 😁

Joking aside, I was curious, do you use Mantine & DataTable internally at namecheap? And if so, do you need help with it? Feel free to drop me a line at the email address listed in my profile if you want.

@gfazioli
Copy link
Contributor Author

First of all, thank you for the wonderful, useful, and beautiful DataTable that has allowed me to create several applications without any problem. 👏

Currently, we are using both Mantine and your Mantine DataTable for an internal application (in Namecheap) that is not visible externally. Unfortunately, due to time constraints, we have not yet migrated to v7, which is planned for 2024.

As you mentioned, open-source should be supported, if possible financially, otherwise, as I have done, with ingenuity.
I would be happy to contribute, as I also have open-source projects on GitHub and therefore I know exactly what it means to maintain them. So you can count on me. 👍

btw, I wanted to wish you a wonderful Christmas and a fantastic 2024 to you and your loved ones 🍾

Thank you again for your passionate work 😉

@icflorescu
Copy link
Owner

Thanks a lot!

Currently, we are using both Mantine and your Mantine DataTable for an internal application (in Namecheap) that is not visible externally.

Would it be ok, then, if I add a logo/link to Namecheap in the docs website footer where I list companies/projects using Mantine DataTable? 🙏

This would help the project a lot, as it encourages people to give it a shot. And it should also help Namecheap with some traffic too.

Wonderful Christmas to you as well! 🍾

@gfazioli
Copy link
Contributor Author

Would it be ok, then, if I add a logo/link to Namecheap in the docs website footer where I list companies/projects using Mantine DataTable? 🙏

I think so, but let me ask our department to be sure 😉. I'll let you know asap

@gfazioli
Copy link
Contributor Author

@icflorescu Good news!
Feel free to mention Namecheap as the company, you can also use the logo.

It would be nice if you mentioned the product we developed using Mantine and Mantine DataTable. The product is called EasyWP, here are some useful links below.

@icflorescu
Copy link
Owner

Will do, thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
code quality Improve code readability documentation Improvements or additions to documentation enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants