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 👍
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.
It's the approach we use in the main repository. It avoids back and forth with the top of the file when refactoring.
https://github.com/dtassone/fin-ui-demo/blob/7f55b34d1927f219ffade1c30d81d1b864c32991/.prettierrc.js#L1-L7 to:
I was reading the code and noticed that most of the lines were going off the screen. 180 chars per line is a lot 😬.
https://github.com/dtassone/fin-ui-demo/blob/7f55b34d1927f219ffade1c30d81d1b864c32991/src/components/grid/grid.tsx#L16
pxis the default unit:https://github.com/dtassone/fin-ui-demo/blob/7f55b34d1927f219ffade1c30d81d1b864c32991/src/components/grid/grid.tsx#L42
componentsin 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.https://github.com/dtassone/fin-ui-demo/blob/7f55b34d1927f219ffade1c30d81d1b864c32991/src/components/grid/styled-wrappers/data-container.tsx#L12 What's the objective?
https://github.com/dtassone/fin-ui-demo/blob/7f55b34d1927f219ffade1c30d81d1b864c32991/src/components/grid/hooks/utils/useLogger.ts#L19
I like the idea behind https://basecamp.com/shapeup/.
onScrolllistener. Would it be worth looking at pruning the logic for the x or y-axis if it wasn't scrolled?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 👍