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

[Feature Request] Less coupled {List|Edit|Show|Create}Controller(s) for customRoutes #2903

Closed
kopax opened this issue Feb 20, 2019 · 10 comments

Comments

@kopax
Copy link
Contributor

kopax commented Feb 20, 2019

We use a lot of customRoutes in our application and we want to display resources data in it.

The {List|Edit|Show|Create}Controller are tightly coupled to the URL which doesn't permit customRoutes to use them correctly.

This is why we are interested to create custom controllers that would be less coupled, and would let us feed the query manually. We will still have a link to create and edit some resources, but the default filtering will be different, and the path to retrieve the data might be too.

  • Do you already have a strategy in place for such pages?
  • Do you recommend to retrieve manually the data and perform that mapping manually?

It will become a requirement for us, If you are interested in such a component to be added into ra-core/ra-ui-materialui, I can probably try to make a proposition soon.

@djhi
Copy link
Collaborator

djhi commented Feb 20, 2019

Well, actually we did get the same issue on a heavily customized react-admin app. We don't have any plans yet but this is definitely on our radar. Feel free to share how you would implement that or even submit a PR!

@kopax
Copy link
Contributor Author

kopax commented Feb 20, 2019

Great, I will get back with a proposal soon.

@fzaninotto
Copy link
Member

Thanks for the suggestion.

ListController uses the URL to initialize filters, sort and pagination, and allows users to share URLs and use the back button.

We want that functionality, so the coupling with the location is necessary.

I don’t want to add another level of indirection to make ListController dépend not on Location but on a wrapper - the code is already complex enough.

So for your use case, I suggest you write a custom controller. You can publish it as a standalone package if you want, but we won’t add it to the core.

@fzaninotto
Copy link
Member

It seems we disagree with @djhi, so I reopen this issue to continue the discussion.

@fzaninotto fzaninotto reopened this Feb 20, 2019
@kopax kopax changed the title [Feature Request] A less coupled ListConstroller for customRoutes [Feature Request] A less coupled ListController for customRoutes Feb 20, 2019
@kopax
Copy link
Contributor Author

kopax commented Feb 20, 2019

Thanks for both opinions. To be honest I am not familiar enough to simply tell how it should be done, if it should be done with an existing class and how it can have side effect with the rest of the react-admin. I am asking advices.

All I know is adding this feature will make much sens for all <Create />, <Edit /> <Show /> and <List /> controllers.

I agree the code is quite complex, perhaps creating a separate component is the best solution.
We can try to clarify what need to be done in order to succeed.

With the existing source, here's the few I can see:

ListController uses the URL to initialize filters

For initialization, we can do this into ListController:

- function mapStateToProps(state, props) {
+ function mapStateToProps(state, { query, ...props }) {
  const resourceState = state.admin.resources[props.resource];
  return {
-    query: selectQuery(props),
+    query: query || selectQuery(props),
    params: resourceState.list.params,
    ids: resourceState.list.ids,
    selectedIds: resourceState.list.selectedIds,
    total: resourceState.list.total,
    data: resourceState.data,
    isLoading: state.admin.loading > 0,
    filterValues: resourceState.list.params.filter,
    version: state.admin.ui.viewVersion,
  };
}

sort and pagination

For this, we only have one resource state per resource, if you want to display multiple time the same entity in the same page, it may cause issue.

If I got it right, state.admin.resouce[props.resource] is a buffer used between the request response and redux form.

I am not sure how this will behave if we keep it like this, can the buffer conflict if multiple list are present?

and allows users to share URLs and use the back button.

The back button and user to share url is because we store the query filter state into the url params, in this version, we could try to disable the params usage from the URL params and use a new state in redux store made for that purpose.

@kopax kopax changed the title [Feature Request] A less coupled ListController for customRoutes [Feature Request] A less coupled {List|Edit|Show|Create}Controller for customRoutes Feb 20, 2019
@kopax kopax changed the title [Feature Request] A less coupled {List|Edit|Show|Create}Controller for customRoutes [Feature Request] Less coupled {List|Edit|Show|Create}Controller(s) for customRoutes Feb 20, 2019
@fzaninotto
Copy link
Member

The problem is that all list actions (filtering, sorting, paginating) are redirections, and the URL is the single source of truth in the ListController as it's written. What you are describing requires altering the component in many ways, and it's in essence, another ListController. You'll have to make choices about what to track in the URL and what not to track (and break the back button feature). And what may make sense for your use case (e.g. displaying several lists in a page) may not make sense for others.

In fact, your use case may be closer to a ReferenceManyFieldController (which only stores the list state internally, but not in the URL) than to the ListController. As you can see, the code of the ReferenceManyFieldController is much smaller and much easier to cope with.

So instead of trying to modify the ListController, try to create a new one. We'll see if the result adds enough value to be added to the core.

@fzaninotto
Copy link
Member

From my point of view, this RFC ends up with: "we're not convinced, please open a PR when you're confident you have something good".

We need to keep the number of open issues low to keep a good mental health, so I'm going to close this one. We'll continue the discussion when you open a PR ;)

@kopax
Copy link
Contributor Author

kopax commented Feb 21, 2019

Sure, I am still working on it. I will get back when I will have something for you. Thanks.

@kopax
Copy link
Contributor Author

kopax commented Mar 18, 2019

@djhi

Well, actually we did get the same issue on a heavily customized react-admin app.

I wonder because of your comment, how do you create more than one list views for an entity with the current react-admin? This is common in backoffice application without calling heavily customized. I assumed you create many resources but #3020 proved me I was wrong.

@fzaninotto
Copy link
Member

FYI, this has been implemented in #5741

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

No branches or pull requests

3 participants