feat: improve datatable

[aldbr]

diracx-web-components tables are managed by tan stack. It is used to:

• select rows
• resize columns
• hide/show columns
• pagination

tan stack also provides a library to virtualize tables but it does not seem to nicely work with MUI Table, so I kept react virtuoso.

I modified the stories accordingly and added some integration tests to make sure the main features of the Job Monitor keeps working.

[ryuwd]

Looks like very sensible refactoring and improvements throughout. I have only small suggestions about style and the login UI

[ryuwd]

Suggested change

	label="Select a Group"
	label="Select your Virtual Organization"

Unless the terminology Virtual Organisation/VO is outdated, it might be better to be consistent so people don't get confused

[aldbr]

The Virtual Organization is selected before:

diracx-web/packages/diracx-web-components/components/Login/LoginForm.tsx

Line 168 in 00b3ecb

label="Select Virtual Organization"

Here user can select the group within the Virtual Organization (e.g. VO: LHCb, groups: [lhcb_user, lhcb_admin]).

Though I could add a a in Select Virtual Organization (I would not add your since a given physical person can be part of different VOs).

What do you think?

[ryuwd]

I completely misread - that's what I get for skim-reading - apologies :D sounds good

[ryuwd]

Suggested change

	Login through your Identity Provider
	Login via your Identity Provider

this is just a suggestion

[aldbr]

Thanks, I will apply it 🙂

[ryuwd]

would it be better just to import useCallback ? again just a style suggestion

[aldbr]

From what I understand (and please correct me if I am wrong), there is no clear advantage of one way against the other (React.use... vs use...) so it's mostly a question of personal preference.

Now it's true that we write both React.use... and use... throughout the code and it would be better to be consistent. Though, if we choose one way I think it would be better to enforce it with an automated check using ESLint for instance (because else I am pretty sure this will be forgotten in a few months/years).

I don't find any easy way to enforce that but I will better check, thanks!

[ryuwd]

similarly here

[ryuwd]

how about something like this?

Suggested change

	export function FilterToolbar<T extends Record<string, unknown>>(
	props: FilterToolbarProps<T>,
	) {
	const {
	columns,
	filters,
	setFilters,
	appliedFilters,
	handleApplyFilters,
	handleClearFilters,
	} = props;
	export function FilterToolbar<T extends Record<string, unknown>>(
	{
	columns,
	filters,
	setFilters,
	appliedFilters,
	handleApplyFilters,
	handleClearFilters,
	}: FilterToolbarProps<T>,
	) {

[aldbr]

From what I understand (again, don't hesitate to correct me if you think I am wrong), this also comes down to personal preference most of the time. Now in our case, I think the structured props has a few advantage over the "destructured" one:

• in your suggestion, you did not add the types of the parameters, and they would make the signature less readable given the number of parameters.
• the props and the comments associated are used by Storybook to provide an interactive documentation about the components for the communities that will have to develop their extensions (e.g. LHCb), I am not sure (I have not checked though) Storybook would be able to read the parameters from there.

Again, no matter our final choice, may be it would deserve a consistency check? (I think there is a destructuring check for that)

[ryuwd]

you did not add the types of the parameters

I'd thought that i'd specified the types (line 44) }: FilterToolbarProps<T>

No biggie if you prefer the current form - I'd thought it might be a nicer way to write out the component props