Skip to content

Conversation

Meklo
Copy link
Contributor

@Meklo Meklo commented May 13, 2024

Inspired by this PR
Major changes :

  • Usage of AG Grid instead of MUI Table for the directory content table : here's the PR adding the needed component to commons-ui
  • CustomAGGrid component have been tweaked in order to use an alternate theme in order to better match material design
  • The logic for retrieving and enriching the table's data from the has been moved to a dedicated hook : useDirectoryContent.ts
  • Columns definitions have been moved to a dedicated utils file : directory-content-utils.ts
  • Renderers for each columns have been moved to a dedicated file : directory-content-renderers.tsx
  • The logic around the retrieval of rows selection - mainly needed for contextual menu and content toolbar - have been revamped in order to take advantage of AG Grid API

@Meklo Meklo changed the title Migrate directory content to AGgrid [WIP]Migrate directory content to AGgrid May 13, 2024
@Meklo Meklo changed the title [WIP]Migrate directory content to AGgrid Migrate directory content to AGgrid May 14, 2024
@Meklo Meklo requested a review from klesaulnier May 15, 2024 07:36
Copy link
Contributor

@klesaulnier klesaulnier left a comment

Choose a reason for hiding this comment

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

Some bugs detected

  • right clicking an element twice will create multiple contextual menus
  • I have a case where a study is stuck in "creation in progress" : before, the type was empty for them, now I have an untranslated "FILTER" label displayed, which causes warning in console
  • editing a description now prompts this warning in console : Warning: React does not recognize the adornmentPosition prop on a DOM element
  • when using the app in french, a enormous amount of translation warning appear in console

Tests: OK

Need to check :

  • alternative theme
  • editing description button is now working on the whole column and can confuse user

Comment on lines 508 to 524
const onGridReady = useCallback(({ api }) => {
api?.sizeColumnsToFit();
}, []);

const getRowId = useCallback((params) => params.data.elementUuid, []);
const getRowStyle = useCallback((cellData) => {
const style = { fontSize: '1rem' };
if (
![ElementType.CASE, ElementType.PARAMETERS].includes(
cellData.data?.type
)
) {
style.cursor = 'pointer';
}
return style;
}, []);

Copy link
Contributor

Choose a reason for hiding this comment

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

can all those methods be created out of the component instead ? It would prevent the usage of useCallback

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I externalized those in a CustomAGGgrid wrapper dedicated for the directory content, it's called DirectoryContentTable.
It reduces boilerplate code inside directory content imo, wdyt ?

setActiveElement(null);
setOpenDescModificationDialog(false);
}}
updateElement={updateElement}
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't understand, does that mean it did not work before ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yup, wasn't supposed to be in the PR and it was fixed it the meantime

@flomillot
Copy link
Contributor

Don't forget to delete the react-virtualized dep added by this PR : #429
Also check that it's removed from package-lock.

@klesaulnier
Copy link
Contributor

There is a bug where there are many elements in a directory
A scroll bar appears, but the bottom elements can't be seen

@Meklo Meklo requested a review from klesaulnier June 5, 2024 16:13
@Meklo Meklo requested a review from achour94 June 6, 2024 08:45
@achour94
Copy link
Contributor

achour94 commented Jun 7, 2024

The custom aggrid table in RHF section does not render correctly

Hugo Marcellin added 2 commits June 10, 2024 16:47
@Meklo Meklo merged commit f8e8d1b into main Jun 10, 2024
@Meklo Meklo deleted the feat/directory_content_aggrid branch June 10, 2024 15:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants