mui-x,mui

[data grid] Implement Column spanning

Benchmark

[core] Bundling vs dependencies

In its current form, the released packages bundle code instead of relying on a nested dependency tree.

For instance:

The @material-ui/x package bundles @material-ui/x-license & @material-ui/x-grid & @material-ui/x-tree-view
The @material-ui/x-grid package bundles @material-ui/x-grid & @material-ui/x-license
The @material-ui/x-tree-view package bundles @material-ui/x-tree-view & @material-ui/x-license

I can't help myself asking, wouldn't it be better to leverage transitive dependencies? In this mode, we would get this tree structure:

   @material-ui/x
    ├── @material-ui/x-grid
    │   └── @material-ui/x-license
    └── @material-ui/x-tree-view
        └── @material-ui/x-license

Which can avoid duplicates in developer's bundle when they start to do fancy stuff, like importing from @material-ui/x and @material-ui/x-tree-view at the same time.

Continuous Integration

A list of checks we will need to run in the CI in order to save us time during development and help guarantee a high code quality:

static analysis
- yarn no diff / deduplicate (#107)
- eslint
- prettier no diff
- lint JSON (#108)
- typescript no errors (.spec.ts) (#187)
publishing
- build (rollup)
- release (codesandbox-ci) https://ci.codesandbox.io/status/mui-org/material-ui/pr/22195/builds/46193 (#227)
tests
- test coverage (#3873)
- test browsers (#115)
- puppeteer (#151, #1443)
misc
- deploy storybook (#116)
- visual screenshots (#1081)
- bundle size diff

A lot should be copied from https://github.com/mui/material-ui, the CI has been gone through numerous iterations there already. It will be much better to compound on it. Then, once we double the size of the team, I think that it could starts to make sense to diverge, exploring different directions.

Early feedback

This issue was originally opened in https://github.com/dtassone/fin-ui-demo/issues/3, 10/04/2020, work was transferred to this repository, for now.

Notes I have taken while looking at the source. Some are more relevant than others. I have been as exhaustive as I could.

I think that we can gain efficiency by replacing https://github.com/dtassone/fin-ui-demo/blob/7f55b34d1927f219ffade1c30d81d1b864c32991/src/components/grid/grid.tsx#L1 with

import * as React from 'react';

It's the approach we use in the main repository. It avoids back and forth with the top of the file when refactoring.

If we could use the same prettier config as the mono-repository, it would help to switch between projects. from
https://github.com/dtassone/fin-ui-demo/blob/7f55b34d1927f219ffade1c30d81d1b864c32991/.prettierrc.js#L1-L7 to:

module.exports = {
  printWidth: 100,
  singleQuote: true,
  trailingComma: 'all',
  overrides: [
    {
      files: '*.d.ts',
      options: {
        // This is needed for TypeScript 3.2 support
        trailingComma: 'es5',
      },
    },
  ],
};

I was reading the code and noticed that most of the lines were going off the screen. 180 chars per line is a lot 😬.

I see that you have started to design an initial API. While not the priority, I would love to get your feedback on the API I started to implement in the past (from #18872).
Regarding the size of the files. I'm looking at the models as an example. I'm more in the camp of having 1,000 LOCs per file, I don't mind, quite the opposite, I love how easy it makes refactoring and searching. My only point is that if you break the models into different files so I could more easily navigate the source, it's not needed.
forwardRef to add at some point ;)
https://github.com/dtassone/fin-ui-demo/blob/7f55b34d1927f219ffade1c30d81d1b864c32991/src/components/grid/grid.tsx#L16
You can assume px is the default unit:
https://github.com/dtassone/fin-ui-demo/blob/7f55b34d1927f219ffade1c30d81d1b864c32991/src/components/grid/grid.tsx#L42

-<DataContainer ref={gridRef} style={{ minHeight: renderCtx?.totalHeight + 'px', minWidth: internalColumns.meta.totalWidth + 'px' }}>
+<DataContainer ref={gridRef} style={{ minHeight: renderCtx?.totalHeight, minWidth: internalColumns.meta.totalWidth }}>

Love the style-wrapper approach. I was thinking of making a components in v5 to enable developers to inject custom versions. I think that it will be interesting to look at the API of https://github.com/DevExpress/devextreme-reactive, it's a prior-work to solve the theming problem.
I doubt we need to force a display name on the elements, for instance:
https://github.com/dtassone/fin-ui-demo/blob/7f55b34d1927f219ffade1c30d81d1b864c32991/src/components/grid/styled-wrappers/data-container.tsx#L12 What's the objective?
A note on arrow function vs named function. We try to follow this approach: named function for top-level scope, arrow function for nested scopes. Could it work for you?
https://github.com/dtassone/fin-ui-demo/blob/7f55b34d1927f219ffade1c30d81d1b864c32991/src/components/grid/hooks/utils/useLogger.ts#L19
Jira account created. I'm in favor of a methodology where we make bets on specific topics with a fixed deadline. So more scrum than kanban.

I like the idea behind https://basecamp.com/shapeup/.

There is a bunch of effect dependency warnings. I don't think that we should ignore any. I'm happy to help with any of them.
In the onScroll listener. Would it be worth looking at pruning the logic for the x or y-axis if it wasn't scrolled?
We might have a 1px offset computation issue. Scroll at the bottom, nothing happens in the first 10% of the scrollbar range.
Does the grid support IE 11? Should we ignore it? (I wonder with position sticky)
If you are only using debounce from lodash, you could import it from import { debounce } from '@material-ui/core/utils'; It's a 10 LOCs method. Speaking of debounce, we should clear it when you unmount the component, otherwise, we risk crashing the app.

Great start 👍

[RFC] Use same tsconfig.json as main repository

It appears we already have a tsconfig.json file in the main repository that differs from the one here, let's make sure we share the same rules.

[DataGrid] Implement Row editing

Benchmark

[XGrid] Column resizing broken in StrictMode

I opened up a new react project to test x-grid and it works for the most part, but resizing doesn't seem to work at the moment.

I've set up my project like this:

import React from 'react';

import { XGrid, RowsProp, ColDef, LicenseInfo } from '@material-ui/x-grid';

LicenseInfo.setLicenseKey(test);

const rows: RowsProp = [
  { id: 1, col1: 'Hello', col2: 'World' },
  { id: 2, col1: 'XGrid', col2: 'Awesome' },
  { id: 3, col1: 'Material-UI', col2: 'Amazing' },
];

const columns: ColDef[] = [
  { field: 'id', hide: true},
  { field: 'col1', headerName: 'Column 1', resizable: true },
  { field: 'col2', headerName: 'Column 2', resizable: true },
];

function App() {
  return (
    <div className='App' style={{ height: 400, width: '100%', display: 'flex', flexGrow: 1 }}>
      <XGrid columns={columns} rows={rows} />
    </div>
  );
}

export default App;

The drag button does seem to work but nothing happens. I also get this error inside the console when i try to drag the columns:

index-esm.js:15 Uncaught TypeError: Cannot read property 'field' of undefined
    at fr (index-esm.js:15)
    at index-esm.js:264
    at HTMLDivElement.<anonymous> (index-esm.js:264)

[data grid] Implement Excel export

Benchmark

Server-side rendering support

Rending the grid on the server fails on:

https://github.com/mui-org/material-ui-x/blob/da5e83aa69291821e14218184d05a2c1c313bfd6/packages/grid/x-grid-modules/src/hooks/utils/useLogger.ts#L4

ReferenceError: localStorage is not defined

This makes me also wonder. Do we want to bundle the useLogger package in production? Is there a way we could make is a noop when process.env.NODE_ENV === 'production'?

[DataGrid] Implement Column reorder

Docs placeholder

https://material-ui.com/components/data-grid/columns/#column-reorder

Benchmark

[RFC] Use Babel

In order to build on top of the same foundation and to save time, I think that we should reuse the existing build steps of the main repository by leveraging Babel. It appears that reproducing the same building pipeline will provide the maximum consistency. For instance

we would like to remove the requirement from yarn link & yarn build in order to work on the documentation or the storybook.
we will likely want to keep the same production optimizations: https://github.com/mui-org/material-ui/blob/2ed7b23dc35cf72265065506e406427dbcf06a77/babel.config.js#L35.
we would like to rely on our browserlist to guarantee browser support, something TypeScript doesn't support microsoft/TypeScript#19183
we don't want to minimize the source in the npm package

Running "npm run start" fails to compile

I pulled the latest changes and after running the project locally it fails to compile. Running the build also doesn't work.

The issue I'm getting is related to missing next folder inside the docs folder.

The commit from which this problem started is this one -> #144

PS: If this is not the right way to create issues please point it out and I'll update it.

[DataGrid] Prefix exported types and modules

I think that we should make sure that all the exported types and modules are unique within the Material-UI ecosystem, this includes all the packages in @material-ui/*. We had a first discussion around the issue on https://github.com/mui-org/material-ui-pickers/issues/1778.

This is important for two reasons:

Avoid all name conflict when importing, as we will re-export everything from @material-ui/x.
Avoid all confusion when searching on Google, in-site Algolia, or even inside your own codebase.

We can benchmark with competitors:

https://www.telerik.com/kendo-react-ui/components/grid/api/: prefix is Grid
https://devexpress.github.io/devextreme-reactive/react/grid/docs/guides/grouping/: no prefix
https://ej2.syncfusion.com/react/documentation/api/grid/: no prefix

I don't think that Kendo is the leader by chance, they have put a lot more care into the design of their components. I think that Grid would make a great prefix. In https://github.com/mui-org/material-ui, we already do our best to avoid conflicts

[DataGrid] Implement Quick filter

Benchmark

Should logger log in process.env.NODE_ENV === test?

Noticed it with oliviertassinari/material-ui@9a3c143.

[RFC] Change API state

Looking at the current shape of the API, it seems that we have an opportunity to explore a more intuitive and flexible API around how the different states can be handled. We have started to uncover this aspect in #133, where we talk about renaming sortModel to sortBy. We have also uncovered some of the problems in #165 (comment) with the update columns method.

Changes I would propose:

It doesn't make sense to have a controllable prop and an imperative method that can update a state at the same time. It's confusing, it can lead to nasty bugs when the grid rerenders with the controllable prop and undo the imperative calls. If we take the sortModel state, I think that setSortModel should warn when used without the uncontrollabe API: defaultSortModel.
The corollary is that we should introduce a defaultSortModel prop to allow the uncontrollable API to function with the imperative API setSortModel.
Update of the defaultSortModel prop could be supported, and equivalent to calling setSortModel.

This gives us:

sortModel: the controllable prop, will be used no matter what as a source of truth.
defaultSortModel: the uncontrollable prop, the state is hosted by the component
setSortModel: imperative API to update the uncontrollable mode, update the state hosted by the component. Throw or warn if the state is already controlled.
onSortModelChange: callback method used for the controllable mode.
getSortModel: imperative API to pull the state hosted by the component.

Another corollary is that we should remove duplicated APIs, for instance: https://github.com/mui-org/material-ui-x/blob/91b05ee8b6cc9fb1beab96187fd4308d4b7b294b/packages/grid/x-grid-modules/src/models/colDef/colDef.ts#L56-L63 should be removed for simplicity and clarity.

In the documentation, I think that we should:

rely on the uncontrollable mode when possible
have the controllable mode documented before the imperative API

While in this demonstration, I take the "sortModel" state as an example, I think that we can apply the very same model to all the other states. In the core package, there is a useControlled() hook to normalize the behavior. Example of other states we can expose:

columnsWidth/defaultColumnsWidth/onColumnsWidthChange e.g. https://devexpress.github.io/devextreme-reactive/react/grid/docs/guides/column-resizing/#controlled-mode
columnsOrder/defaultColumnsOrder/onColumnsOrderChange e.g. https://devexpress.github.io/devextreme-reactive/react/grid/docs/guides/column-reordering/#controlled-mode

This should solve my concerns from #165 (comment). Exposing a resetState publicly doesn't feel right, not at all. From what I understand the boolean influences: caching and internal states. Caching should stay private and internal states should be exposed independently (columnsWidth).

[DataGrid] Rename sortModel to sortBy

This change aims to make the objective of the prop easier to grab and memorize. It's pretty similar to what most ORMs have, matching the ORDER BY command of SQL. First discussed in #130 (comment).

Discrepancy with main repository

The purpose of this issue is to identify misalignment between the main repository and this one, see what improvement we can bring in any direction.

While it's unclear how this package will evolve in the future, staying standalone or merging with the main repository as we plan for the pickers. So far the great reason not to merge is to keep the effort private and allow fast POC iterations. But what about once we make it public? I think that our default answer to the problem would be to go all-in in the mono-repository approach unless there is a great reason not to, something that would outweigh the cost (like we have right now).

The core idea around why this approach can help bring value:

Put constraints in place to maximize the consitency of the developer experience on the whole library (user side).
Make it easier for an engineer to contribute to the library, as a whole, no matter which component we are used to working on. Experimentations should go through small efforts / RFC and be rollout at 100% when successful, not scattered between components.
Share knowledge, tools, and great practices learned the hard way. Any small incremental improvement to the CI, tests, etc compound. It can be very hard to share and enforce different repositories.

List of items

Left is mui-org/material-ui-x, right is mui-org/material-ui. I will make the case for why the right approach might be better at the same time. I would love to hear counter-arguments in the other direction.

[RFC] Remove dependency on @material-ui/icons

The line I'm going after:

https://github.com/mui-org/material-ui-x/blob/fb5af45b5d74ca818fc6945af17400cec0ee21a2/packages/grid/package.json#L17

While there are a couple of leverages we can use to make a hard dependency on @material-ui/icons more viable, I don't think that it's a wise direction to take. I would propose we apply the same tradeoff than with all the other packages. We inline the few SVG icons we need. It's a small bundle size duplication overhead. To the best of my knowledge, we wouldn't have a lot of them. Here are examples:

Replace enzyme with testing-library

We are moving away from enzyme to testing-library in the main repository. 💯 for starting writing tests with the best tools available: testing-library 👌. Meaning replacing:

https://github.com/mui-org/material-ui-x/blob/fb5af45b5d74ca818fc6945af17400cec0ee21a2/packages/grid/jest/setupTest.ts#L1-L4

Color Accessibility threshold

Potential issue

https://muix-storybook.netlify.app/?path=/story/real-data-demo--commodity

I see two issues in the above case:

The color contrast of the checkbox is 1.39:1 https://webaim.org/resources/contrastchecker/?fcolor=F50057&bcolor=4A98EC. It fails to match the WCAG AA minimum accessibility target (4.5:1).
Luckily, Google has us covered 🙂 https://material.io/design/interaction/states.html#selected

Potential solution

I think that we can solve the above problems with:

Use the primary color for the Checkbox
Patch the selected background color until we update the value to match the specification in the core. 8% of the opacity of the primary in light mode and 16% in dark mode.

[DataGrid] Implement Column pinning

Benchmark

https://ant.design/components/table/#components-table-demo-fixed-columns

[DataGrid] Implement CSV export

We have started to work on the feature a year ago, the person that takes on this task can refer to mui/material-ui#18872 and this code: saveAs.

Research

UIs inspiration I could find:

Google Ads

The "More options" button opens a modal where you can select the format of the CSV file.

Airtable

Retool

Redash

Stripe

https://try.firetable.io/table/mvpResources

Benchmark

[data grid] Implement Column groups

Grouping columns allows you to have multiple levels of columns in your header and the ability, if needed, to 'open and close' column groups to show and hide additional columns.

Benchamark

Column group collapse

https://glideapps.github.io/glide-data-grid/?path=/story/glide-data-grid-dataeditor-demos--column-group-collapse

Re-evaluate requirement for tslib as a dependency

This is a follow-up on a conversation we have started in #44 (comment). The tslib dependency was added in order to make codesandbox runs successfully. It's unclear if the requirement is coming from codesandbox itself or from a misconfiguration of the building pipeline.

It's also to be noted that we use babel-runtime on @material-ui/core which might duplicate in the bundle for our users.

@dmtrKovalenko Considering we have a similar building stack on the pickers, do you have any idea of what could be wrong? :)

I have added the "Post MVP" milestone as we can/should delay the resolution to later.

Improve CSS architecture

3 improvements opportunities:

Make specificity as low as possible. I have discovered this looking at the component, there are cases where we will need to allow the developer to customize the rendering of the component, we should identify these cases and make sure the CSS specificity is as low as possible. There are specificity over 5 (barely overridable) in:
https://github.com/mui-org/material-ui-x/blob/3a68cef50b0906674f3e43b37bd888c2e7b4c86c/packages/grid/x-grid-modules/src/components/styled-wrappers/grid-root.tsx#L11
The higher the specificity is the harder the overrides are.
Differentiate class names that are public and private. It's unclear, looking with the Chrome dev tool, which classes are private, and which ones are public to be customized.
For the name of the class names, let's use the same convention as the main repository.
Reduce bloat. I think that we should drop styled-components for the first MVP to simplify the usage of the grid (one less dependency to install & to configure for SSR & injection order), leverage the values from the theme (easier overrides), reduce the bundle size (-12 kB gzipped).

Add React.forwardRef to all the public components

The list of public components I have noticed missing the forward ref logic

Grid. A reproduction: https://codesandbox.io/s/trusting-vaughan-voq8o. The source:

https://github.com/mui-org/material-ui-x/blob/fb5af45b5d74ca818fc6945af17400cec0ee21a2/packages/grid/src/grid.tsx#L24

… no more I'm aware of, Grid seems to be the only public component for now.

[RFC] Consider removing constraint on memoization of the params of the data grid

I'm taking notes on #134 (comment) in case the topic comes up again, in the future.

[data grid] Implement Clipboard import

Summary

Copy and paste data between an Excel sheet and a Data Grid.

Benchmark

[RFC] Rename xxxClass props to xxxClassName

The reflection started at #43 (comment), opening an issue so we can keep track of it.

[DataGrid] Remove dependency on lodash

While lodash was inlined in the repository (I guess to hide, on the surface, the dependency on this library), I don't think that it removes the fact that we depend on it. I would be in favor of completely removing its modules from the codebase.

Possible migration plan:

denounce. Instead of the 938 B version of lodash we could rely on the 305 B import { denounce } from @material-ui/core module. We could also move the module to @material-ui/utils instead of the core if that makes more sense. This module also has the advantage of being deduplicated with the core components, sharing the simple source.
deep equal. Instead of the 3.8 Kb version of lodash, we could replace it with fast-deep-equal, a 414 B alternative. I have only seen one use case in the codebase, I even wonder if we couldn't use JSON.stringify directly.

https://github.com/mui-org/material-ui-x/blob/fb5af45b5d74ca818fc6945af17400cec0ee21a2/packages/grid/src/hooks/root/useColumns.tsx#L162

[RFC] Increase the scope of React peer dependencies down to 16.8.0

For consistency with the core packages and compatibility with the ecosystem, I think that we can widen the scope of the support versions of React from ^16.13.1 to ^16.8.0.

I'm not aware of any downsides to doing such, only upside with consistency against the other Material-UI's packages. The day we get a bug from somebody that uses an older version and we can fix by increasing the minimum version of React supported, sure.

priority: small, cost: small

[DataGrid] Implement Print export

Benchmark

[docs] Opening the data grid demos in codesandbox doesn't work

We would need to:

Remove the peer dependency on styled-components: #99
Improve the generation of the package.json's dependency logic to handle @material-ui/xy correctly and assume that @material-ui/core should be installed.

[RFC] Should we host MUI X on the main repository?

The idea to host the X line product in the main repository is growing in me. While we are going in this direction for the pickers: mui/material-ui#19706, I would like to continue the reflection for X. It's not obvious.

Pros

Better integration and coherence between the MIT and paid content: more code shared, more knowledge shared.
Avoid doing back and forth between the source and the documentation. From what I understand, the workflow will be as follow: make the change in this repository, wait for the next release schedule, (1-week frequency?) once the code is released, go to the main repository, update the documentation, wait for the next release schedule (1-week frequency). Live.
Share the same tooling, I have started to look at the tooling gap. I believe #23 and #24 are 5% of what's left to address. CircleCI, codesandbox-ci, bundle size tracking, test coverage, visual regression, Netlify deployment, cross-browser end to end tests (need to replace Jest with Mocha).
Else?

Cons

Less audience isolation. We risk paid content perverting and negatively influencing the MIT community.
Slower CI, which is the price for better integration. We can mitigate it by spending $ on better machines ($0/month so far) and introducing parallelism.
Else?

Can it scale?

Looking at larger projects, yes, it seems so

Note that if bug tracking is a concern, we can very well manage the concern independently to where the source is hosted: fullcalendar/fullcalendar-workspace#558.

Prior-arts

Ag-grid
- https://github.com/ag-grid/ag-grid (MIT)
- https://github.com/ag-grid/ag-grid-enterprise (Paid) But moved back to the main one
Full calendar
- https://github.com/fullcalendar/fullcalendar (MIT)
- https://github.com/fullcalendar/fullcalendar-scheduler (Paid)
Else?

cc @mui-org/core

setState leak after unmount

@dtassone The issue was caught in the CI (Karma), something is calling setState after the component is unmounted. It leaks.

@eps1lon this makes me think that we might have an improvement opportunity in the CI, shouldn't we fail if a component call console.warn or console.error during the tests? I believe it works when we run the tests in JSDOM, but not with Karma. I don't know why yet. The data grid might be an exception as all JSDOM tests are skipped (require layout).

[DataGrid] Bundle size is way too large

The current bundle size and bundling strategy is not flying. It's not living up to the quality level our audience expects from the core components. https://bundlephobia.com/result?p=@material-ui/data-grid

Tree-shaking

First, tree-shaking is not working. (tested with CRA). This means that, for instance, developers bundle all the locales. I think that the solution is to drop the current bundling strategy and copy what the core is doing. I think that this bundling problem is hard enough, we don't have the resources to solve the problems twice because we use two different solutions between different packages.

in

import * as React from 'react';
import { GridToolbar } from '@material-ui/data-grid';

export default function DataGridDemo() {
  console.log('GridToolbar', GridToolbar)
  return null;
}

out, in the production bundle

Inefficiencies

Second, here is a couple of bundle size-reduction opportunities:

clsx. (win ~0.3 kB gzipped) Replace classnames with clsx.
license. (win ~0.7 kB) I believe we can remove the need from including packages/license/src/encoding/base64.ts in the bundle of the users. The possible solution would work as follow:
- client-side, use the built-in window.atob and window.btoa, only run the license check if the window is defined (ignore server-side, and CI)
- server-side, us the built-in Buffer.from('Hello World!').toString('base64'); and Buffer.from(b64Encoded, 'base64').toString() utils.
lodash. The dependency was inlined in the repository (I guess to hide, on the surface, the dependency on this library), I don't think that it removes the fact that we depend on it. I would be in favor of completely removing its modules from the codebase.
Recommended migration plan:
denounce. (win ~ (0.5 kB) Instead of the 938 B version of lodash we could rely on the 305 B import { denounce } from @material-ui/core module. We could also move the module to @material-ui/utils instead of the core if that makes more sense. This module also has the advantage of being deduplicated with the core components, sharing the simple source.
deep equal. (win ~3.5 kB gzipped) Instead of the 3.8 Kb version of lodash, we could replace it with fast-deep-equal, a 414 B alternative. I have only seen one use case in the codebase, I even wonder if we couldn't use JSON.stringify directly (see #3954)
Select. (win ~0.5 kB gzipped) Replace Select import with NativeSelect).
TextField (win ~5 kB gzipped). Replace TextField with individual imports, allow dropping OutlinedInput and FilledInput from the bundle..
Babel (win ~0.6 kB gzipped) Remove propTypes in production with this Babel plugin.

https://github.com/mui-org/material-ui-x/blob/fb5af45b5d74ca818fc6945af17400cec0ee21a2/packages/grid/src/hooks/root/useColumns.tsx#L162

[RFC] Use event.key over event.code

A follow-up on #115 (comment).

[XGrid] Column resizing is too limiting

I believe that extending the resizing session up to the mouse up event would improve the UX. I don't think that we should interrupt the resizing session when the mouse leaves the column header. This surface area is relatively small. It would be better for end-users not to have to concentrate on keeping the same vertical alignment with their pointing device. It also feels weird to have the mouse pressed but not doing anything (once escaping the zone).

KO

OK

Cells typography

Potential issue

I'm going to assume that we want to build a first theme for the preview of the DataGrid. In this context, we want to follow the Material Design specification: https://material.io/design/components/data-tables.html. I have seen the following divergences with the specification: the typography:

Check the implementation of TableCell for a version that is supposed to (unless proven otherwise 🙃) matches the specification.

Potential solution

The solution would be about:

using theme.typography.body2
using theme.palette.text.primary
loading the Roboto font instead of using the system font

Actually, I wonder. Would the performance be good enough if we used the component? (I suspect it won't be but it would be interesting to figure that out. Alternatively, would it make sense to only import the style of the component?

[core] Improve packages folder?

@dtassone What about we:

Rename packages/license folder -> packages/x-license-pro to match package name
Flatten the packages/grid folder to make it easier to navigate the codebase?

Remove React.memo() anytime a react element is accepted

While the default strategy should be to not use React.memo() unless there is a good reason to use it cc @eps1lon, looking at the codebase, there is a quick win on the problem. In 99% of the cases, anytime a React element is accepted as a prop, React.memo() is a waste of CPU cycles (unless this babel plugin kicks in, which is not frequent).

Components we can make faster be removing memo:

[RFC] Package structure

This is a follow-up on a couple of past discussions we had, mostly speaking. I couldn't find any written conclusion on how to best name the package and components outside of a verbal discussion that no other member can introspect. In order to improve decision making and transparency, I'm making a written RFC, so we can settle.

Introduction

We came to the conclusion at https://github.com/mui-org/material-ui-pickers/issues/1778 that:

All the components exposed in Material-UI are in a single namespace.
How we structure the npm packages should be only driven by "deliver" concerns. So far:
- isolating MIT and enterprise licenses, legal concerns: @material-ui/core vs @material-ui/x.
- testing if it helps increase market share, also help to provide KPI to assert the quality of our work: @material-ui/pickers.
- share code between multiple packages: @materal-ui/utils.

Problems

Looking at the current naming organization, I see a couple of things we could potentially improve:

The Grid component name is already reserved by https://material-ui.com/api/grid/. We can't use it here.
I believe it would be better not to have an intermediary /grid folder. A flat structure would be easier to navigate.
It seems that the intermediary /grid was introduced to fix a naming organization issue: if we sort ASC, related components won't be grouped. This might be an issue we would want to solve userland too (not only in the repository).

Solutions?

A couple of options that we could consider to solve the above:

We could put emphasis on the components, paid or not. This would enable to flatten the file structure. Moving from:

/data-grid
/x-grid-data-generator
/x-grid-modules
/x-grid
/pickers
/x-pickers
/x-tree-view

to

/data-grid
/grid-x-generator
/grid-x-modules
/grid-x
/pickers
/pickers-x
/tree-view-x

I've not convinced it's better, but it could have its advantages, to consider.

We could simply remove the intermediary /grid folder. I think that it would make it easier to navigate the codebase.
We could consider that the Enterprise version of the component is meant to completely replace and extend the MIT version. I think that it would create a good constraint for the developer experience. We would only document one component, with props that are restricted to X with a dedicated notice.
In practice, it would mean:

import { DatePicker } from '@material-ui/pickers';
import { DateRangePicker } from '@material-ui/x-pickers';
// import { DataGrid } from '@material-ui/data-grid';
import { DataGrid } from '@material-ui/x-data-grid';

In the even 3. it not identified as the best approach, we can differentiate the two components (free and paid):

import { DatePicker } from '@material-ui/pickers';
import { DateRangePicker } from '@material-ui/x-pickers';
import { DataGrid } from '@material-ui/data-grid';
import { XGrid } from '@material-ui/x-grid';

[DataGrid] Implement multi-column filter

Plan of action

1. Build a column header menu, it will be used for filtering, sorting and any other feature that a column can leverage.

2. Build the internal state filtering logic to support one type: string and one operator: contain.
3. Create a global filter UI popup, same as Airtable, Notion, Retool.

4. Expose a programmatic public filtering API (3. was for end-users, 4. is for developers).
5. Support all the operations and types possible: date, date range, exclude, etc.
6. Add a toolbar component #516 (filter popup open, display density, etc.)

7. Implement quick filter #202

8. Consider supporting inline filter (e.g. react-table demo) but we might not do it.

Benchmark

Screenshots

https://react-table.tanstack.com/docs/examples/filtering

https://ant.design/components/table/#components-table-demo-head

https://material-table.com/#/docs/features/filtering

https://ag-grid.com/javascript-grid-filtering/

https://ej2.syncfusion.com/react/demos/#/material/grid/filter-menu

https://js.devexpress.com/Demos/WidgetsGallery/Demo/DataGrid/Filtering/React/Light/

https://www.jqwidgets.com/react/react-grid/#https://www.jqwidgets.com/react/react-grid/react-grid-filtering.htm

http://tabulator.info/docs/4.5/filter

https://www.bryntum.com/examples/grid/filterbar/

https://handsontable.com/examples?manual-resize&manual-move&conditional-formatting&context-menu&dropdown-menu&headers&filters

https://www.infragistics.com/products/ignite-ui-angular/angular/components/treegrid/tree_grid.html

https://baseweb.design/components/unstable-data-table/#batch-actions

https://try.firetable.io/table/mvpResources

Bundle issue

Something is wrong with the integration of the grid with the main repository:

The grid crashes on the main documentation if you try to render it

The grid crashes on the main testing suite if you try to render it:

[DataGrid] Implement Cell editing

Benchmark

This feature is also related to the editable row #204.

Exported types should live in a global namespace

I have noticed the following import when reading the documentation that made me wonder

https://github.com/mui-org/material-ui-x/blob/3318a2792c0e92ca9e5f09f6129c75b8ca526641/packages/storybook/src/documentation/pages/demos/columns/columnTypes.demo.tsx#L2

Shouldn't we assume that all public API lives in a global namespace? https://github.com/mui-org/material-ui-pickers/issues/1778. Let's assume we have two components that have an Columns exported variable.

We plan to reexport all the components from @material-ui/x. I expect that most developers will use it for simplicity. Conflict
What about people that want to search? In their codebase, in Google, in the built-in search of the documentation?

I would recommend:

-import { Columns, RowsProp, XGrid } from '@material-ui/x-grid'; 
+import { GridColumns, GridRowsProp, XGrid } from '@material-ui/x';

Default boolean prop value should be false

Baseline rule: https://material-ui.com/guides/api/#property-naming

Issues:

Surface code review

Here are some of my foundings on the current material-ui-x codebase.

Do not take it personally, just some of my observations

Not using clsx module to combine class names

Such expressions are so unreadable (https://github.com/mui-org/material-ui-x/blob/master/packages/grid/src/components/cell.tsx#L6):

let cssClasses = `${CELL_CSS_CLASS} ${cssClass || ""} ${
  showRightBorder ? "with-border" : ""
}`;

cssClasses += !align || align === "left" ? "" : ` ${align}`;

Classname expressions className={'country-flag'} are a little bit weird.
Which is potentially caused by 1.
The function component should have return all the time because any potential change that will require variable or state hook in the component will require to change the whole component code. Which harms git history.

export const IsDone: React.FC<{ value: boolean }> = React.memo(({ value }) =>
  value ? <DoneIcon fontSize={"small"} /> : <ClearIcon fontSize={"small"} />
);

export const IsDone: React.FC<{
  value: boolean;
}> = React.memo(({ value }) => {
  return value ? (
    <DoneIcon fontSize={"small"} />
  ) : (
    <ClearIcon fontSize={"small"} />
  );
});

Do we really need to wrap all the components in React.memo? Memoization costs runtime performance. And probably we do not need to make premature optimizations:

MOST OF THE TIME YOU SHOULD NOT BOTHER OPTIMIZING UNNECESSARY RERENDERS. React is VERY fast and there are so many things I can think of for you to do with your time that would be better than optimizing things like this. In fact, the need to optimize stuff with what I'm about to show you is so rare that I've literally never needed to do it in the 3 years I worked on PayPal products and the even longer time that I've been working with React.
cc https://kentcdodds.com/blog/usememo-and-usecallback

Avoid using inline styles style={{ display: 'flex', justifyContent: 'space-between' }} (https://github.com/mui-org/material-ui-x/blob/master/packages/grid-data-generator/src/renderer/incoterm-renderer.tsx, https://github.com/mui-org/material-ui-x/blob/master/packages/grid-data-generator/src/renderer/progress-bar.tsx and other)
Grid cell field prop must be well-typed string union which will autocomplete the other available props (https://github.com/mui-org/material-ui-x/blob/master/packages/grid/src/components/cell.tsx#L6)
Make sure that we are using strict mode in tsconfig.json. I am seeing some functions without argument types, that would be inferred as any
Expressions like icons: { [key: string]: React.ReactElement }; are so unsafe. key must be well-typed for autocomplete and type system experience
(https://github.com/mui-org/material-ui-x/blob/master/packages/grid/src/components/column-headers.tsx#L13)
Probably we need some convention about the body of the functional component. IMHO it is much more readable when all the state/ref hooks are going first, then declaring effects, then callbacks and than return. It is similar to what we have with class components (1. state in the constructor, 2. methods, and the last render method) E.g. this component is really hard to read.
Avoid using 1-letter variable names, even for maps {columns.map((c, idx) => ( (https://github.com/mui-org/material-ui-x/blob/master/packages/grid/src/components/column-headers.tsx#L94)
This .map loop is to long and too nested. Really hard to read, probably need to split in some reusable functions to improve readability.
Do we really need constants for css classes? Looks like it is too different from material-ui styling solution (https://github.com/mui-org/material-ui-x/blob/master/packages/grid/src/constants/cssClassesConstants.ts)
Events must be properly typed MouseEvent is native dome event. React.MouseEvent is react synthetic event and must be used as React.MouseEvent<HTMLElement> to prevent explicit casts later on and improve typings. (https://github.com/mui-org/material-ui-x/blob/master/packages/grid/src/hooks/root/useApi.ts#L58

Overall typescript doesn't work well right from what I am seeing. For example probably we want to use import * as React from 'react' in this project as well to get better access to the react typings.

Probably we don't need to use explicit type definitions everywhere like here:

  const getColumnFromField: (field: string) => ColDef = field => stateRef.current.lookup[field];
  const getAllColumns: () => Columns = () => stateRef.current.all;
  const getColumnsMeta: () => ColumnsMeta = () => stateRef.current.meta;
  const getVisibleColumns: () => Columns = () => stateRef.current.visible;

We could make the return value to be inferred automatically. The same for anonym objects.

It is used ! operator really rarely which means that we have an unsound type system and we are going to crash here 😱 just like here. Probably we want to use ? operator?

Unnecessary spaces between imports. Used a lot, example from https://github.com/mui-org/material-ui-x/blob/master/packages/grid/src/hooks/root/useKeyboard.ts

import { useEffect } from 'react';

import { useLogger } from '../utils/useLogger';
import { KEYDOWN_EVENT, KEYUP_EVENT, MULTIPLE_KEY_PRESS_CHANGED } from '../../constants/eventsConstants';

import { GridApiRef } from '../../grid';

Do we really need to use forceUpdate's in each hook? Probably we can avoid using it?
Do we need to duplicate ColumnsContainer.displayName = 'ColumnsContainer'; for each component? It will be inferred by react automatically
Is it intended that we are using global classnames? I suppose we need to remove usage of the global class names like here. It is potentially dangerous because of accidental overrides. But I suppose it will be redone.
Probably we need to use prettier for code formatting, to increase readability for other contributors.

mui / mui-x Goto Github PK

mui-x's People

Contributors

Stargazers

Watchers

Forkers

mui-x's Issues

Benchmark

Benchmark

Benchmark

Benchmark

Docs placeholder

Benchmark

Benchmark

List of items

Potential issue

Potential solution

Benchmark

Research

Benchmark

Benchamark

Column group collapse

Summary

Benchmark

Benchmark

Pros

Cons

Can it scale?

Prior-arts

Tree-shaking

Inefficiencies

Potential issue

Potential solution

Introduction

Problems

Solutions?

Plan of action

Benchmark

Benchmark

Recommend Projects

Recommend Topics

Recommend Org