-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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: removes array
& blocks
& group
fields from sort
#6574
Conversation
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.
Can't we just simplify the components so they don't have all this logic in them? I would love it if field.disableSort
were the source of truth for the disable prop.
@@ -29,6 +29,8 @@ const SortColumn: React.FC<Props> = (props) => { | |||
const descClasses = [`${baseClass}__desc`] | |||
if (sort === desc) descClasses.push(`${baseClass}--active`) | |||
|
|||
const shouldRenderButtons = !disable && type !== 'array' && type !== 'group' && type !== 'blocks' |
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.
In my opinion the SortColumn shouldn't have to know about the specifics between different types of fields. It already gets a disabled
prop which should be enough to know whether to include the controls or not.
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.
@DanRibbens so field.disableSort
is not a real prop, it really just needs to be removed - the condition would never be hit by anything.
However, I do agree that there should be just one source of truth - aka the disable
prop
I've went ahead and updated the PR to essentially transferring the conditional to buildColumns and pass it into the disable
prop there.
Let me know what you think.
packages/payload/src/admin/components/elements/TableColumns/buildColumns.tsx
Outdated
Show resolved
Hide resolved
## Description V2 PR [here](#6574) - [x] I have read and understand the [CONTRIBUTING.md](https://github.com/payloadcms/payload/blob/main/CONTRIBUTING.md) document in this repository. ## Type of change - [x] Bug fix (non-breaking change which fixes an issue) ## Checklist: - [x] Existing test suite passes locally with my changes
## Description V2 PR [here](#6574) - [x] I have read and understand the [CONTRIBUTING.md](https://github.com/payloadcms/payload/blob/main/CONTRIBUTING.md) document in this repository. ## Type of change - [x] Bug fix (non-breaking change which fixes an issue) ## Checklist: - [x] Existing test suite passes locally with my changes
Description
Fixes #6469
Type of change
Checklist: