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 incorrect <Ready> page when dynamically load routes with no resources #8490

Merged
merged 8 commits into from
Dec 15, 2022
Merged

Fix incorrect <Ready> page when dynamically load routes with no resources #8490

merged 8 commits into from
Dec 15, 2022

Conversation

smeng9
Copy link
Contributor

@smeng9 smeng9 commented Dec 10, 2022

As a follow up of feature #7609 under the use case of defining an admin without resources but routes,

There seems to be a bug if additional routes are lazily added to the list of child routes when user navigates in the app.

It triggers a rerender https://github.com/marmelab/react-admin/blame/master/packages/ra-core/src/core/useConfigureAdminRouterFromChildren.tsx#L123 and it incorrectly redirects the user to <Ready /> component

This pull requests adds additional checks similar to https://github.com/marmelab/react-admin/blame/master/packages/ra-core/src/core/useConfigureAdminRouterFromChildren.tsx#L253

@smeng9
Copy link
Contributor Author

smeng9 commented Dec 10, 2022

Needs to fix implementation of <Layout> for ra-no-code because if there are any <CustomRoutes> in

const InnerAdmin = (props: AdminProps) => {
the <Ready> with import CSV pop up will not be shown. (which now should be the correct behavior, previous behavior is incorrect.) The <Ready> needs to be moved to <Layout>

Copy link
Contributor

@slax57 slax57 left a comment

Choose a reason for hiding this comment

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

Overall your fix seems good, thanks!

But it would be nice to have some tests to prove that it works, and cover this case in the future. Could you add some?

packages/ra-no-code/src/ui/Layout.tsx Show resolved Hide resolved
@smeng9
Copy link
Contributor Author

smeng9 commented Dec 14, 2022

But it would be nice to have some tests to prove that it works, and cover this case in the future. Could you add some?

I have added a test case so the initial state of lazyRoutes is empty and later is set to a route by useEffect. It triggers a rerender of the useConfigureAdminRouterFromChildren hook.

@smeng9 smeng9 requested a review from slax57 December 14, 2022 05:58
Copy link
Contributor

@slax57 slax57 left a comment

Choose a reason for hiding this comment

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

Also, there is a linter warning in packages/ra-no-code/src/Admin.tsx line 15:

'Ready' is defined but never used

Copy link
Contributor

@slax57 slax57 left a comment

Choose a reason for hiding this comment

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

Thanks!

@slax57 slax57 added this to the 4.6.2 milestone Dec 15, 2022
@slax57 slax57 merged commit 3c4d367 into marmelab:master Dec 15, 2022
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