-
-
Notifications
You must be signed in to change notification settings - Fork 5.3k
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
Tabbed components disable routing #5945
Conversation
I merged |
docs/CreateEdit.md
Outdated
@@ -870,6 +870,7 @@ Here are all the props accepted by the `<TabbedForm>` component: | |||
* `saving`: A boolean indicating whether a save operation is ongoing. This is passed automatically by `react-admin` when the form component is used inside `Create` and `Edit` components. | |||
* [`warnWhenUnsavedChanges`](#warning-about-unsaved-changes) | |||
* [`sanitizeEmptyValues`](#setting-empty-values-to-null) | |||
* [`syncWithLocation`](#syncwithlocation) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think the anchor matches the autogenerated section id
@@ -73,7 +74,9 @@ const Tab = ({ | |||
value={value} | |||
icon={icon} | |||
className={classnames('show-tab', className)} | |||
{...({ component: Link, to: value } as any)} // to avoid TypeScript screams, see https://github.com/mui-org/material-ui/issues/9106#issuecomment-451270521 | |||
{...(syncWithLocation |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I find it weird that you handle the tab click here when syncWithLocation
is true, and in the parent component when it's false.
I think you'd better move the tab click handling in the parent component altogether.
Same in formTab.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's because it's a simple link when syncWithLocation
is true
. I'd rather avoid having buttons calling the router methods as it is less accessible imo
|
||
export const getTabFullPath = (tab, index, baseUrl) => | ||
`${baseUrl}${ | ||
tab.props.path ? `/${tab.props.path}` : index > 0 ? `/${index}` : '' | ||
}`; | ||
}`.replace('//', '/'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what's that for? a comment would help
location.pathname !== value, | ||
})} | ||
component={syncWithLocation ? Link : undefined} | ||
to={{ ...location, pathname: value }} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
does the to
prop make sense if syncWithLocation
is false?
import TextInput from '../input/TextInput'; | ||
import { fireEvent } from '@testing-library/react'; | ||
import { fireEvent, isInaccessible, waitFor } from '@testing-library/react'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Linter warning
@@ -69,12 +76,13 @@ export const getTabFullPath = ( | |||
): string => | |||
`${baseUrl}${ | |||
tab.props.path ? `/${tab.props.path}` : index > 0 ? `/${index}` : '' | |||
}`; | |||
}`.replace('//', '/'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same, please explain
Co-authored-by: Francois Zaninotto <francois@marmelab.com>
ecf1bb8
to
fa3a4c3
Compare
tests fail |
No description provided.