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

Replaced search with state on CloneButton #4421

Closed
wants to merge 3 commits into from

Conversation

akshah123
Copy link
Contributor

Now that issue # 301 is fixed on connect-react-router, we can upgrade the package and start using state instead of search in react-admin.

This should address #4066

@akshah123
Copy link
Contributor Author

I still see the problem after switching from search to state in clone. Likely problem is caused by something else. 😞

@akshah123
Copy link
Contributor Author

I am thinking this has to do with switch from redux-form to react-final-form.

@fzaninotto
Copy link
Member

Yes, there is a bug in the tab navigation. Please add the following changes:

--- packages/ra-core/src/sideEffect/useRedirect.ts
+++ packages/ra-core/src/sideEffect/useRedirect.ts
@@ -4,7 +4,7 @@ import { useDispatch } from 'react-redux';
 import { Identifier, Record } from '../types';
 import resolveRedirectTo from '../util/resolveRedirectTo';
 import { refreshView } from '../actions/uiActions';
-import { useHistory } from 'react-router-dom';
+import { useHistory, useLocation } from 'react-router-dom';
 
 type RedirectToFunction = (
     basePath?: string,
@@ -32,6 +32,7 @@ export type RedirectionSideEffect = string | boolean | RedirectToFunction;
 const useRedirect = () => {
     const dispatch = useDispatch();
     const history = useHistory();
+    const location = useLocation();
     return useCallback(
         (
             redirectTo: RedirectionSideEffect,
@@ -40,13 +41,17 @@ const useRedirect = () => {
             data?: Partial<Record>
         ) => {
             if (!redirectTo) {
-                dispatch(refreshView());
+                if (location.state) {
+                    history.replace({ ...location, state: {} });
+                } else {
+                    dispatch(refreshView());
+                }
                 return;
             }
 
             history.push(resolveRedirectTo(redirectTo, basePath, id, data));
         },
-        [dispatch, history]
+        [dispatch, history, location]
     );
 };
 
--- packages/ra-ui-materialui/src/form/FormTab.js
+++ packages/ra-ui-materialui/src/form/FormTab.js
@@ -1,6 +1,6 @@
 import React from 'react';
 import PropTypes from 'prop-types';
-import { Link } from 'react-router-dom';
+import { Link, useLocation } from 'react-router-dom';
 import MuiTab from '@material-ui/core/Tab';
 import classnames from 'classnames';
 import { useTranslate } from 'ra-core';
@@ -35,6 +35,7 @@ const FormTab = ({
     ...rest
 }) => {
     const translate = useTranslate();
+    const { state } = useLocation();
 
     const renderHeader = () => (
         <MuiTab
@@ -44,7 +45,7 @@ const FormTab = ({
             icon={icon}
             className={classnames('form-tab', className)}
             component={Link}
-            to={{ pathname: value }}
+            to={{ pathname: value, state }}
             {...sanitizeRestProps(rest)}
         />
     );

@fzaninotto
Copy link
Member

wait, my fix is wrong: it'll reset the changes made in other tabs. I'll push a good fix in another branch - the upgrade of connected-react-router should be independent.

@fzaninotto
Copy link
Member

#4422 fixes the problem. Your PR, which changes the clone vector from search to state, should still work once rebased on master after we merge #4422.

@fzaninotto fzaninotto changed the base branch from master to next February 14, 2020 15:18
@fzaninotto fzaninotto changed the base branch from next to master February 14, 2020 15:18
@fzaninotto
Copy link
Member

Now, this isn't a fix since #4422 fixed the problem. This is a feature change, though, as the CloneButton now uses state instead of search. Would you mind rebasing on next? I tried doing it on Github but I got too many extra commits.

@akshah123
Copy link
Contributor Author

@fzaninotto Thank you for fixing the underlying issue. I will work on getting this rebased of next.

@akshah123 akshah123 changed the base branch from master to next February 14, 2020 15:31
@fzaninotto
Copy link
Member

I think your rebase got wrong - this PR embarks commits that weren't yours. Could you please try again?

Now that issue marmelab#301 is fixed on connect-react-router, we can upgrade the package and start using state instead of search in react-admin.
@akshah123
Copy link
Contributor Author

@fzaninotto Take a look now. I do see more changes than what I had expected in the yarn.lock file. However, I am not sure if that's expected or if I missed something.

@fzaninotto
Copy link
Member

indeed. Do you mind not committing the yarn.lock? I'll commit the one generated by my checkout based on your packages.json.

@akshah123 akshah123 force-pushed the clone_state_fix branch 2 times, most recently from b980cc4 to 8ee609a Compare February 21, 2020 19:03
@akshah123
Copy link
Contributor Author

@fzaninotto done!

@fzaninotto
Copy link
Member

This PR needs rebase

@fzaninotto fzaninotto closed this Jun 5, 2020
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.

2 participants