Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow to configure export button props from list component and global admin #8285

Closed
soullivaneuh opened this issue Oct 20, 2022 · 8 comments

Comments

@soullivaneuh
Copy link
Contributor

soullivaneuh commented Oct 20, 2022

Is your feature request related to a problem? Please describe.

We currently need to change the default export max result from 1000 (defined here) to 5000 globally on the app.

To do so, we have first to create custom List and ListActions components:

const ListActions: FC<ListActionsProps> = ({ filters, ...props }) => {
  const {
    sort,
    filterValues,
    exporter,
    total,
  } = useListContext(props);
  const resource = useResourceContext(props);
  const { hasCreate } = useResourceDefinition(props);

  return useMemo(
    () => (
      <TopToolbar>
        <FilterButton
          filters={Array.isArray(filters) ? filters : [filters]}
        />
        {hasCreate && <CreateButton />}
        {exporter !== false && (
          <ExportButton
            disabled={total === 0}
            resource={resource}
            sort={sort}
            filterValues={filterValues}
            // The only thing we want to override is that part.
            maxResults={5000}
          />
        )}
      </TopToolbar>
    ),
    [
      exporter,
      filterValues,
      filters,
      hasCreate,
      resource,
      sort,
      total,
    ],
  );
};

export const List: FC<ListProps> = ({ children, filters, ...props }) => (
  <ListBase {...props}>
    <ListToolbar
      filters={filters}
      actions={<ListActions />}
    />
    <Card>
      {children}
      <Pagination />
    </Card>
  </ListBase>
);

Then, we have to replace the vendor List usage by our local one on each list resource we define on our project (nearly 15 for our case).

I see two main drawbacks with that:

  1. This is cumbersome. We have now 50 lines of custom component just to change one number prop.
  2. By using a custom one, we lose the benefit of simply use the provided vendor list and its furthers update because of the override.

Describe the solution you'd like

The ability to define the exporter option directly to the List:

<List
  export={{
    maxResults: 5000,
  }}
>

Or globally using the Admin component:

<Admin
  export={{
    maxResults: 5000,
  }}
>

Describe alternatives you've considered
The alternative is already described by the problem section.

Additional context
N/A

@slax57
Copy link
Contributor

slax57 commented Oct 21, 2022

Thanks for the suggestion.

Another possibility would be to add Configurable capabilities to the ExportButton, leveraging #8145, so that you can set the maxResults in local preferences.

@fzaninotto what do you think?

@fzaninotto
Copy link
Member

Hi @soullivaneuh, and thanks for your suggestion.

Replacing import { List } from 'react-admin' with import { List } from '../common' doesn't feel like a big enough pain point to justify the added complexity in the react-admin code (passing the configuration down via a context or via cloning). Besides, having the configuration lifted up in a "god" component goes against one of the principles guiding the react-admin design( see https://marmelab.com/react-admin/Architecture.html#component-composition).

So we won't allow to customize the export size in the <Admin> or <List> components.

As for making the <ExportButton> configurable, I think it may help in some situations, but not for @soullivaneuh's problem.

@soullivaneuh
Copy link
Contributor Author

@fzaninotto You are right, replacing the vendor import by a local one is not a big pain, and this is not what I reported.

The big pain to me, is the fact we have to copy/paste all the vendor code of List and ListActions just to be able to change the maxResult prop of the ExportButton, residing on ListActions.

This is what is represented by the code block added to the issue body: dozens of line just to change one, plus the fact we do not rely to the future vendor update anymore.
This is the cumbersome pain I wanted to report here.

I understand the principles you pointing our, but I think the problematic is still justified.

Can this issue be reconsidered and discussed about other possible alternatives?

Regards

@WiXSL
Copy link
Contributor

WiXSL commented Oct 22, 2022

Maybe is not a crazy idea to add maxResults as an optional prop to ListActions component and pass it to the ExporButton.
So one has to pass <ListActions maxResults={5000}> to a List component

@soullivaneuh
Copy link
Contributor Author

Yes, at least that if you do not want a super prop on the Admin instance.

Even if this is more configuration tweaking here, so we should have a way to easily access to that.

Even I understand we should avoid having one component with a ton of props, but it also make no sense to me to completely override core component of react-admin just to be able to change one prop of a sub component being used on it.

@fzaninotto
Copy link
Member

@soullivaneuh You shouldn't have to write that much code to override the ListActions

const ListActions = () => (
    <TopToolbar>
        <FilterButton/>
        <CreateButton/>
        <ExportButton/>
    </TopToolbar>
);

Is all you need.

https://marmelab.com/react-admin/List.html#actions

@soullivaneuh
Copy link
Contributor Author

@fzaninotto Well, your sample is indeed tiny, but does not look like the vendor one:

export const ListActions = (props: ListActionsProps) => {
const { className, filters: filtersProp, hasCreate: _, ...rest } = props;
const {
sort,
displayedFilters,
filterValues,
exporter,
showFilter,
total,
} = useListContext(props);
const resource = useResourceContext(props);
const { hasCreate } = useResourceDefinition(props);
const filters = useContext(FilterContext) || filtersProp;
return useMemo(
() => (
<TopToolbar className={className} {...sanitizeListRestProps(rest)}>
{filtersProp
? cloneElement(filtersProp, {
resource,
showFilter,
displayedFilters,
filterValues,
context: 'button',
})
: filters && <FilterButton />}
{hasCreate && <CreateButton />}
{exporter !== false && (
<ExportButton
disabled={total === 0}
resource={resource}
sort={sort}
filterValues={filterValues}
/>
)}
</TopToolbar>
),
/* eslint-disable react-hooks/exhaustive-deps */
[
resource,
displayedFilters,
filterValues,
filtersProp,
showFilter,
filters,
total,
className,
sort,
exporter,
hasCreate,
]
);
};

And you get rid of all the dynamic button logic addition, which its need to make our custom component re-usable across the resource, plus the useMemo getting ride off (not necessary? 🤔)

So no, it's a bit more complicated than that in my opinion.

@fzaninotto
Copy link
Member

The react-admin code is more complex because we have to maintain backward compatibility with previous versions. You only have one version to manage.

Anyway, let's agree to disagree. You want to write less code, we want to avoid complexity, and these two objectives can't meet in this feature request. Considering that your problem already has a solution, I suggest we leave it here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants