refactor: convert DataGridHeader into function component #24747

ArturBa · 2023-10-06T15:04:25Z

Summary

Convert DataGridHeader into function component

Ticket Link

Screenshots

Release Note

NONE

mattermost-build · 2023-10-06T15:04:30Z

Hello @ArturBa,

Thanks for your pull request! A Core Committer will review your pull request soon. For code contributions, you can learn more about the review process here.

jespino

It looks great! Thanks @ArturBa! ✨

larkox

Great work! Just two small polishing changes.

larkox · 2023-10-06T16:32:05Z

webapp/channels/src/components/admin_console/data_grid/data_grid_header.tsx

-class DataGridHeader extends React.Component<Props> {
-    renderHeaderElement(col: Column) {
+
+const DataGridHeader: React.FC<Props> = ({ columns }: Props) => {


The explicit type should not be needed.

larkox · 2023-10-06T16:33:23Z

webapp/channels/src/components/admin_console/data_grid/data_grid_header.tsx

+
+    return (
+        <div className='DataGrid_header'>
+            {columns.map((col) => renderHeaderElement(col))}


Can be simplified

Suggested change

{columns.map((col) => renderHeaderElement(col))}

{columns.map(renderHeaderElement)}

larkox · 2023-10-06T17:09:13Z

@ArturBa I just saw that the linter is failing. Probably just executing npm run fix in the webapp/channels folder should fix those automatically.

larkox

Sorry but it would be great if you could do one extra change. Thanks 😄

larkox · 2023-10-07T11:30:07Z

webapp/channels/src/components/admin_console/data_grid/data_grid_header.tsx

-class DataGridHeader extends React.Component<Props> {
-    renderHeaderElement(col: Column) {
+const DataGridHeader = ({columns}: Props) => {
+    const renderHeaderElement = (col: Column) => {


The renderHeaderElement is a component itself, and doesn't need any of the props, so we could move that out of the component. So... we would have the code something like this:

const HeaderElement = ({col}: HeaderElementProps) => { ... } const DataGridHeader = ({columns}: Props) => { return ( <div className='DataGrid_header'> {columns.map((col) => <HeaderElement col=col>)} </div> ) }

Why this is better? If you declare the function outside of the component, it gets instantiated just once during the lifetime of the program. Otherwise, on every render of every data grid header you will instantiate the function and potentially remove all references at some point, which consumes extra memory and generates extra work for the GC.

Sorry for the back and forth with this. I just saw this approach on a different PR and realized it is something nice to do here too 😄

larkox

Awesome! Thanks!

mattermost-build · 2023-10-07T11:53:11Z

E2E tests not automatically triggered, because the PR is not in a mergeable state. Please update the branch with the base branch and resolve outstanding conflicts.

larkox · 2023-10-09T15:25:48Z

/update-branch

Tahanima · 2023-10-16T17:34:25Z

Hi @lindy65, I would like to test this PR as a QA. Looking forward to your guidance.

lindy65 · 2023-10-17T10:09:42Z

Hi @Tahanima - thank you! I have set up a test server but first want to check with dev that the section in the System Console is, in fact, what I think it is... Also, if you haven't already, please join our Community Server where you'll be able to chat to other staff and community QAs :)

@larkox - do you know whether the data grid header referred to in Files Changed is, in fact, the System Console > Reporting > Team Statistics?

Please also run E2E tests @larkox - thanks!

larkox · 2023-10-17T10:51:01Z

/e2e-test

mattermost-build · 2023-10-17T10:51:05Z

Successfully triggered E2E testing!
GitLab pipeline | Test dashboard

larkox · 2023-10-17T10:52:33Z

@lindy65 All "grid lists" on the admin console should use the the DataGrid component, and therefore the data grid header (top row of the grid showing the column headers).

For example, the users lists, or the log list in the server logs should use this.

lindy65 · 2023-10-17T14:41:42Z

Thanks for the explanation @larkox

@Tahanima - the test server is ready. Log in a the system admin and go to the system console. Please go through all options in the left hand side (so we don't miss and grid type lists) and check that they are displayed correctly. A few examples:

System Console > Reporting > Team Statistics
System Console > User Management > Users

Let me know if you have any questions :)

Tahanima · 2023-10-18T17:40:23Z

I've tested the changes in the test server and it looks good to me.

Steps

Log in to the application as a System Admin user (Username: sysadmin, Password: Sys@dmin123)
Navigate to the System Console
Go through each of the navigational options in the Sidebar and check whether they are displayed correctly.

Operating System

Ubuntu 22.04.3 LTS

Recordings

Web Application

Firefox (Version: 118.0.2)

firefox.webm

Chrome (Version: 118.0.5993.70)

chrome.webm

Desktop Application

desktop.webm

cc: @lindy65

lindy65

Thanks @ArturBa - looks good 👍

Thank you @Tahanima for QA'ing!

cc @larkox

mm-cloud-bot · 2023-10-19T06:55:40Z

Test server destroyed

larkox · 2023-10-19T09:00:31Z

Thank you for the contribution!

…24747) * refactor: convert DataGridHeader into function component * fix: core review * split into 2 components * add key for maped items * lint firx * fix missing parentheses * fix bracket --------- Co-authored-by: Mattermost Build <build@mattermost.com>

refactor: convert DataGridHeader into function component

eb7bfbf

mm-cloud-bot added kind/feature Categorizes issue or PR as related to a new feature. release-note-none Denotes a PR that doesn't merit a release note. labels Oct 6, 2023

mattermost-build added Hacktoberfest Contributor labels Oct 6, 2023

jespino requested review from jespino and larkox October 6, 2023 16:30

jespino approved these changes Oct 6, 2023

View reviewed changes

jespino added the 2: Dev Review Requires review by a developer label Oct 6, 2023

larkox requested changes Oct 6, 2023

View reviewed changes

hanzei added the Awaiting Submitter Action Blocked on the author label Oct 6, 2023

fix: core review

0716ade

jespino requested a review from larkox October 7, 2023 08:04

jespino removed the Awaiting Submitter Action Blocked on the author label Oct 7, 2023

larkox requested changes Oct 7, 2023

View reviewed changes

split into 2 components

08d3e4a

larkox approved these changes Oct 7, 2023

View reviewed changes

larkox added 3: QA Review Requires review by a QA tester. May occur at the same time as Dev Review QA-wanted Available for QA review by QA contributors during Hacktoberfest and removed 2: Dev Review Requires review by a developer labels Oct 7, 2023

ArturBa added 4 commits October 7, 2023 17:37

add key for maped items

d3f02a0

lint firx

19b8d67

fix missing parentheses

ab4f077

fix bracket

a1d5765

Merge branch 'master' into feat/24742

98f9203

larkox assigned lindy65 Oct 9, 2023

larkox requested a review from lindy65 October 17, 2023 08:39

larkox unassigned lindy65 Oct 17, 2023

lindy65 assigned Tahanima Oct 17, 2023

lindy65 added Setup Cloud Test Server Setup an on-prem test server and removed QA-wanted Available for QA review by QA contributors during Hacktoberfest labels Oct 17, 2023

lindy65 approved these changes Oct 19, 2023

View reviewed changes

larkox merged commit ec394c1 into mattermost:master Oct 19, 2023
15 checks passed

amyblais added Changelog/Not Needed Does not require a changelog entry Docs/Not Needed Does not require documentation labels Oct 19, 2023

ArturBa deleted the feat/24742 branch October 21, 2023 10:57

jwilander added the kind/refactor Categorizes issue or PR as related to refactor of production code. label Nov 20, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: convert DataGridHeader into function component #24747

refactor: convert DataGridHeader into function component #24747

ArturBa commented Oct 6, 2023

mattermost-build commented Oct 6, 2023

jespino left a comment

larkox left a comment

larkox Oct 6, 2023

larkox Oct 6, 2023

larkox commented Oct 6, 2023

larkox left a comment

larkox Oct 7, 2023

larkox left a comment

mattermost-build commented Oct 7, 2023

larkox commented Oct 9, 2023

Tahanima commented Oct 16, 2023

lindy65 commented Oct 17, 2023

larkox commented Oct 17, 2023

mattermost-build commented Oct 17, 2023

larkox commented Oct 17, 2023

lindy65 commented Oct 17, 2023

Tahanima commented Oct 18, 2023 •

edited

Loading

lindy65 left a comment •

edited

Loading

mm-cloud-bot commented Oct 19, 2023

larkox commented Oct 19, 2023

	{columns.map((col) => renderHeaderElement(col))}
	{columns.map(renderHeaderElement)}

refactor: convert DataGridHeader into function component #24747

refactor: convert DataGridHeader into function component #24747

Conversation

ArturBa commented Oct 6, 2023

Summary

Ticket Link

Screenshots

Release Note

mattermost-build commented Oct 6, 2023

jespino left a comment

Choose a reason for hiding this comment

larkox left a comment

Choose a reason for hiding this comment

larkox Oct 6, 2023

Choose a reason for hiding this comment

larkox Oct 6, 2023

Choose a reason for hiding this comment

larkox commented Oct 6, 2023

larkox left a comment

Choose a reason for hiding this comment

larkox Oct 7, 2023

Choose a reason for hiding this comment

larkox left a comment

Choose a reason for hiding this comment

mattermost-build commented Oct 7, 2023

larkox commented Oct 9, 2023

Tahanima commented Oct 16, 2023

lindy65 commented Oct 17, 2023

larkox commented Oct 17, 2023

mattermost-build commented Oct 17, 2023

larkox commented Oct 17, 2023

lindy65 commented Oct 17, 2023

Tahanima commented Oct 18, 2023 • edited Loading

Steps

Operating System

Recordings

Web Application

Desktop Application

lindy65 left a comment • edited Loading

Choose a reason for hiding this comment

mm-cloud-bot commented Oct 19, 2023

larkox commented Oct 19, 2023

Tahanima commented Oct 18, 2023 •

edited

Loading

lindy65 left a comment •

edited

Loading