Skip to content
This repository has been archived by the owner on Mar 13, 2024. It is now read-only.

MM-32425 new data table component for data retention #7467

Merged

Conversation

BenCookie95
Copy link
Contributor

@BenCookie95 BenCookie95 commented Feb 3, 2021

Summary

This component already existed in billing history but we needed to re-use it. I pulled it out and made a few adjustments to allow for server side pagination and it is functionally similar to the data grid

See design here: https://www.figma.com/proto/PdZPkf36lNU6ZO41JlEjKd/Team-%26-Channel-Data-Retention?node-id=20%3A181&scaling=min-zoom

Ticket Link

https://mattermost.atlassian.net/browse/MM-32425

Screenshots

Screen Shot 2021-02-03 at 2 21 45 PM

Added a new optional prop to the DataGrid component called customClass. The new custom class called customTable is used by data retention.

@sbishel sbishel requested review from devinbinnie and removed request for sbishel February 4, 2021 19:38
components/admin_console/data_table/data_table.scss Outdated Show resolved Hide resolved
components/admin_console/data_table/data_table.tsx Outdated Show resolved Hide resolved
@hahmadia hahmadia added 1: UX Review Requires review by a UX Designer 2: Dev Review Requires review by a core commiter 3: QA Review Requires review by a QA tester labels Feb 6, 2021
Copy link
Contributor

@hahmadia hahmadia left a comment

Choose a reason for hiding this comment

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

I had one question regarding this component.

We already have a common component called DataGrid which looks very similar to the screenshots you’ve posted and has extra things but they are optional (such as a search bar).

I think we can replace instances of data tables with data grid (with small modifications such as passing custom css class) to achieve the same thing without needing too create a whole new component.

This way, we could help to avoid confusion between differentiating between the two components and when each one should be used.

You can take a look at the component here
https://github.com/mattermost/mattermost-webapp/blob/master/components/admin_console/data_grid/data_grid.tsx

And see it being used in the teams and channel page in the system console (System Console > User Management > Teams/Channels). The grid displaying the teams and channels is using this.

Anyhow, let me know what you think. These were just my thoughts after I realized what it was we were creating. Thanks!

Edit I believe you can also see an instance of the DataGrid component being used in the fashion of a Data Table in either the Users section of the Teams/Channels page in the system console. It just has this extra search functionality added.

@BenCookie95
Copy link
Contributor Author

I had one question regarding this component.

We already have a common component called DataGrid which looks very similar to the screenshots you’ve posted and has extra things but they are optional (such as a search bar).

I think we can replace instances of data tables with data grid (with small modifications such as passing custom css class) to achieve the same thing without needing too create a whole new component.

This way, we could help to avoid confusion between differentiating between the two components and when each one should be used.

You can take a look at the component here
https://github.com/mattermost/mattermost-webapp/blob/master/components/admin_console/data_grid/data_grid.tsx

And see it being used in the teams and channel page in the system console (System Console > User Management > Teams/Channels). The grid displaying the teams and channels is using this.

Anyhow, let me know what you think. These were just my thoughts after I realized what it was we were creating. Thanks!

Edit I believe you can also see an instance of the DataGrid component being used in the fashion of a Data Table in either the Users section of the Teams/Channels page in the system console. It just has this extra search functionality added.

I agree with everything you said, I've integrated it into the datagrid now. Looks a lot better and the updates were pretty minimal!

@BenCookie95
Copy link
Contributor Author

/update-branch

Copy link
Contributor

@hahmadia hahmadia left a comment

Choose a reason for hiding this comment

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

Hey! I had a discussion with @devinbinnie and came to realization that the way we are handling the CSS here can be done in a more efficient manner. He will comment about the required changes to make this happen. Thanks!

Copy link
Contributor

@hahmadia hahmadia left a comment

Choose a reason for hiding this comment

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

LGTM. Agree with Devin NIT. Thanks!

@hahmadia hahmadia removed the 2: Dev Review Requires review by a core commiter label Feb 11, 2021
@BenCookie95 BenCookie95 removed the 1: UX Review Requires review by a UX Designer label Apr 9, 2021
Copy link
Contributor

@furqanmlk furqanmlk left a comment

Choose a reason for hiding this comment

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

image

@BenCookie95 BenCookie95 removed the 3: QA Review Requires review by a QA tester label Apr 13, 2021
@BenCookie95 BenCookie95 merged commit 45f33ac into mattermost:master Apr 13, 2021
@amyblais amyblais added Changelog/Done Required changelog entry has been written Docs/Not Needed Does not require documentation labels Apr 22, 2021
@mm-cloud-bot
Copy link

@BenCookie95: Adding the "do-not-merge/release-note-label-needed" label because no release-note block was detected, please follow our release note process to remove it.

I understand the commands that are listed here

1 similar comment
@mm-cloud-bot
Copy link

@BenCookie95: Adding the "do-not-merge/release-note-label-needed" label because no release-note block was detected, please follow our release note process to remove it.

I understand the commands that are listed here

@amyblais amyblais added Docs/Needed Requires documentation and removed Docs/Not Needed Does not require documentation labels Apr 22, 2021
@amyblais amyblais added this to the v5.36.0 milestone Apr 22, 2021
@amyblais amyblais added Docs/Not Needed Does not require documentation and removed Docs/Needed Requires documentation labels Apr 22, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Changelog/Done Required changelog entry has been written Docs/Not Needed Does not require documentation release-note
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants