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

Tabbed components disable routing #5945

Merged
merged 9 commits into from Feb 24, 2021
Merged

Conversation

djhi
Copy link
Contributor

@djhi djhi commented Feb 23, 2021

No description provided.

@djhi djhi added the RFR Ready For Review label Feb 23, 2021
@djhi djhi added this to the 3.13 milestone Feb 23, 2021
@fzaninotto
Copy link
Member

I merged master on next, so you'll need to rebase your PR.

@@ -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)
Copy link
Member

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

docs/CreateEdit.md Outdated Show resolved Hide resolved
docs/CreateEdit.md Outdated Show resolved Hide resolved
docs/Show.md Outdated Show resolved Hide resolved
docs/Show.md Outdated Show resolved Hide resolved
@@ -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
Copy link
Member

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.

Copy link
Contributor Author

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('//', '/');
Copy link
Member

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 }}
Copy link
Member

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';
Copy link
Member

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('//', '/');
Copy link
Member

Choose a reason for hiding this comment

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

same, please explain

@djhi djhi force-pushed the tabbed-components-disable-routing branch from ecf1bb8 to fa3a4c3 Compare February 24, 2021 08:53
@djhi djhi changed the base branch from next to master February 24, 2021 08:53
@fzaninotto
Copy link
Member

tests fail

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
RFR Ready For Review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants