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

[Doc] Improve ListActions override #6218

Merged
merged 11 commits into from
Jun 17, 2021

Conversation

WiXSL
Copy link
Contributor

@WiXSL WiXSL commented Apr 27, 2021

Implements #6212

@djhi
Copy link
Collaborator

djhi commented Apr 28, 2021

👍 Maybe we should do the same for the other action components as well.

@WiXSL WiXSL changed the title Add children to ListActions component Add children to ListActions, CreateActions, EditActions and ShowActions components Apr 28, 2021
Copy link
Member

@fzaninotto fzaninotto left a comment

Choose a reason for hiding this comment

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

Thanks for your patch. However, I'm not comfortable with this change, and for 2 reasons:

  • it creates 2 ways to do one simple thing. This is confusing, which way should a developer use and why?
  • your API "hides" that some other buttons are included. I think that will lead to WTF moments for developers.

Overall, I don't think we should implement that.

@WiXSL
Copy link
Contributor Author

WiXSL commented Apr 29, 2021

Ok, how about moving the logic of the Actions components to hooks, so you always override the actions prop like now, but having control over default actions by calling these hooks (or not).

Example:

import Button from '@material-ui/core/Button';
import { TopToolbar, Edit, useEditActions } from 'react-admin';

const PostEditActions = (props) => {
     const DefaultActions = useEditActions(props);

    return (
      <TopToolbar>
          <DefaultActions/> 
          {/* Add your custom actions */}
          <Button 
                color="primary"
                onClick={() => { alert('Your custom action'); }}
          >
                Custom Action
          </Button>
      </TopToolbar>
    );
};

export const PostEdit = (props) => (
    <Edit actions={<PostEditActions />} {...props}>
        ...
    </Edit>
);

@WiXSL
Copy link
Contributor Author

WiXSL commented May 17, 2021

@fzaninotto,
What do you think of this idea?

@djhi
Copy link
Collaborator

djhi commented May 17, 2021

Why a hook here? We could just have a component rendering a fragment with the default actions right?

@WiXSL
Copy link
Contributor Author

WiXSL commented May 17, 2021

You are right.
So is it ok to continue with this implementation, using a component instead of a hook?

@fzaninotto
Copy link
Member

When I think about it, only the <ListActions> are hard to override. Maybe this is where we should do the change.

First, the documentation could be simplified since we now use the various context:

const ListActions = (props) => {
    const {
        className,
-       exporter,
-       filters,
-       maxResults,
        ...rest
    } = props;
    const {
-       currentSort,
-       resource,
-       displayedFilters,
-       filterValues,
-       hasCreate,
-       basePath,
-       selectedIds,
-       showFilter,
        total,
    } = useListContext();
    return (
        <TopToolbar className={className} {...sanitizeListRestProps(rest)}>
-           {filters && cloneElement(filters, {
-               resource,
-               showFilter,
-               displayedFilters,
-               filterValues,
-               context: 'button',
-           })}
+           {cloneElement(filters, { context: 'button' })}
-           <CreateButton basePath={basePath} />
+           <CreateButton />
-           <ExportButton
-               disabled={total === 0}
-               resource={resource}
-               sort={currentSort}
-               filterValues={filterValues}
-               maxResults={maxResults}
-           />
+           <ExportButton disabled={total === 0} maxResults={maxResults} />
            {/* Add your custom actions */}
            <Button
                onClick={() => { alert('Your custom action'); }}
                label="Show calendar"
            >
                <IconEvent />
            </Button>
        </TopToolbar>
    );
};

For a simpler use case, the doc is even simpler:

const ListActions = () => (
    <TopToolbar >
        {cloneElement(filters, { context: 'button' })}
        <CreateButton />
        <ExportButton />
        {/* Add your custom actions */}
        <Button
            onClick={() => { alert('Your custom action'); }}
            label="Show calendar"
        >
            <IconEvent />
        </Button>
    </TopToolbar>
);

I think this is simple enough that we don't have to introduce a new component for that.

@WiXSL
Copy link
Contributor Author

WiXSL commented Jun 3, 2021

So, I should revert everything and only update the documentation?

@WiXSL WiXSL changed the title Add children to ListActions, CreateActions, EditActions and ShowActions components [Doc] Improve ListActions override Jun 9, 2021
Copy link
Collaborator

@djhi djhi left a comment

Choose a reason for hiding this comment

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

Thanks!

docs/List.md Outdated Show resolved Hide resolved
docs/List.md Outdated Show resolved Hide resolved
WiXSL and others added 2 commits June 17, 2021 09:25
@WiXSL WiXSL requested a review from djhi June 17, 2021 12:29
docs/List.md Outdated Show resolved Hide resolved
Co-authored-by: Gildas Garcia <[email protected]>
@WiXSL WiXSL requested a review from djhi June 17, 2021 12:41
@djhi djhi requested a review from fzaninotto June 17, 2021 12:51
docs/List.md Outdated Show resolved Hide resolved
packages/ra-ui-materialui/src/list/ListActions.tsx Outdated Show resolved Hide resolved
@WiXSL WiXSL requested a review from fzaninotto June 17, 2021 18:49
@fzaninotto fzaninotto merged commit a170dae into marmelab:master Jun 17, 2021
@fzaninotto
Copy link
Member

Thanks!

@fzaninotto fzaninotto added this to the 3.16.3 milestone Jun 17, 2021
@WiXSL WiXSL deleted the listactions-children branch June 17, 2021 20:23
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.

3 participants