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

Fix custom actions cannot override basePath #3043

Merged
merged 4 commits into from
Apr 2, 2019
Merged

Fix custom actions cannot override basePath #3043

merged 4 commits into from
Apr 2, 2019

Conversation

kopax
Copy link
Contributor

@kopax kopax commented Mar 22, 2019

Previously, it was impossible to write: <Show basePath="/foo" actions={<ShowActions basePath="/foo/bar" />} /> because we were not tolerating props on DefaultActions, making the API/props useless/broken.

Dimitri KOPRIWA added 2 commits March 23, 2019 02:33
Previously, it was impossible to write: `<Show basePath="/foo" actions={<ShowActions basePath="/foo/bar" />} />` because we were not tolerating `props` on `DefaultActions`, making the API/props useless/broken.
So far, it was not possible to edit the DefaultActions props due to this missing spread
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.

Extract from react doc:

Clone and return a new React element using element as the starting point. The resulting element will have the original element’s props with the new props merged in shallowly

I'm quite sure this is not needed.

@kopax
Copy link
Contributor Author

kopax commented Mar 22, 2019

Indeed, it is.

cloneElement does keep props as you mentioned (doc), but createElement 2nd arguments will override any props set beforehand, making impossible to customize basePath, resource and so on.

It's easy to setup a test. I hit the bug and already verified, basePath can now be fully customized. Try to set any of the props you set in your second arguments of createElement and you'll reproduce it. I've applied the fix to the 4 crud routes.

It won't break, because, by default, none of the <DefaultAction /> set those props.

This was my use case, I have a custom route with path /projects/:projectId/services/:serviceId/newsletters/groups/:groupId/contacts/:id/show:

import React from 'react';
import WithPermissions from 'ra-core/lib/auth/WithPermissions';
import ShowActions from 'ra-ui-materialui/lib/detail/ShowActions';
import ProjectServiceNewsletterContactShowDefault from '../../resources/contacts/ProjectServiceNewsletterContactShow';

function ProjectServiceNewsletterContactShow({ roles, match, staticContext, ...rest }) {
  return (
    <WithPermissions
      render={({ permissions }) => (
        <ProjectServiceNewsletterContactShowDefault
          actions={<ShowActions basePath={"/newsletter/groups/contacts"} />}
          hasEdit={permissions.includes(roles.PROJECT_EDIT)}
          id={match.params.id}
          basePath={match.url}
          resource={"newsletter/groups/contacts"}
          {...rest}
        />
      )}
    />
  );
}

export default ProjectServiceNewsletterContactShow;

kopax pushed a commit to bootstrap-styled/ra-ui that referenced this pull request Mar 22, 2019
cloneElement was setting props, and no spread applied after that, was making impossible to modify
them

marmelab/react-admin#3043
Copy link
Contributor Author

@kopax kopax left a comment

Choose a reason for hiding this comment

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

Thanks in advance for review

@djhi
Copy link
Collaborator

djhi commented Mar 24, 2019

Ah I see. Fair point!

@djhi djhi changed the title fix(Show): Allowing actions props to be customized Allow actions props injected by react-admin to be customized Mar 24, 2019
@djhi djhi requested a review from fzaninotto March 28, 2019 07:15
@fzaninotto fzaninotto changed the title Allow actions props injected by react-admin to be customized Fix custom actions cannot override basePath Apr 2, 2019
@djhi
Copy link
Collaborator

djhi commented Apr 2, 2019

There are probably many places where this should be done too

@fzaninotto fzaninotto merged commit 2082038 into marmelab:master Apr 2, 2019
@fzaninotto
Copy link
Member

Thanks!

@fzaninotto fzaninotto added this to the 2.8.5 milestone Apr 2, 2019
@kopax kopax deleted the patch-4 branch April 2, 2019 09:27
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