Early feedback

[oliviertassinari]

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.

1. 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.

2. 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 😬.

3. 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).
4. 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.
5. forwardRef to add at some point ;)
   https://github.com/dtassone/fin-ui-demo/blob/7f55b34d1927f219ffade1c30d81d1b864c32991/src/components/grid/grid.tsx#L16
6. 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 }}>

7. 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.
8. 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?
9. 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
10. 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/.

11. 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.
12. In the onScroll listener. Would it be worth looking at pruning the logic for the x or y-axis if it wasn't scrolled?
13. We might have a 1px offset computation issue. Scroll at the bottom, nothing happens in the first 10% of the scrollbar range.
14. Does the grid support IE 11? Should we ignore it? (I wonder with position sticky)
15. 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 👍

[oliviertassinari]

Closing this issue, I have moved some of the concerns to standalone issues, as well as #6.