Refactor Records Show page to show fields as per view #9354
Conversation
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
PR Summary
This PR implements field sorting logic for the Records Show page, introducing a new hook useSortRecordByView and refactoring the FieldsCard component to display fields in a specific order based on the current view.
- Implementation in
useSortRecordByView.tsdoesn't fully satisfy requirements - missing separation between relation/non-relation fields in sorting logic FieldsCard.tsxneeds to handle relation fields separately to match required order: visible non-relation -> visible relation -> non-visible relation -> non-visible non-relation- Current sorting logic in
useSortRecordByView.tsonly splits fields into visible/non-visible without considering field types - Missing view context preservation for relation field ordering as specified in requirements
- New "Other Fields" section styling matches Figma but placement needs adjustment per field type requirements
2 file(s) reviewed, 3 comment(s)
Edit PR Review Bot Settings | Greptile
| const orderedFields = visibleTableColumns | ||
| .map((column) => | ||
| fieldMetadataItems.find((item) => item.id === column.fieldMetadataId), | ||
| ) | ||
| .filter((fieldMetadataItem): fieldMetadataItem is FieldMetadataItem => | ||
| Boolean(fieldMetadataItem), | ||
| ); |
There was a problem hiding this comment.
logic: This implementation only handles visible vs non-visible fields, but doesn't separate relation fields from non-relation fields as required in the issue
| const remainingFields = fieldMetadataItems | ||
| .filter( | ||
| (item) => | ||
| !visibleTableColumns.some( | ||
| (column) => column.fieldMetadataId === item.id, | ||
| ), | ||
| ) | ||
| .sort((a, b) => a.label.localeCompare(b.label)); |
There was a problem hiding this comment.
logic: Non-visible fields need to be split into relation and non-relation fields, with relations maintaining view order and non-relations sorted alphabetically
packages/twenty-front/src/modules/object-record/record-show/components/FieldsCard.tsx
Outdated
Show resolved
Hide resolved
|
Great work!!! Thanks a lot @harshrajeevsingh Haven't checked the code yet but it works! When I tried it, it raised a few design questions: If no view is passed then we should use the field order from the default All view, current behavior: For related objects we might want to show the fields from the default All view, current behavior: For kanban view do we want to always display the kanban field at the top? it feels a bit odd to have it at the bottom while it's the most important field, current behavior: cc @Bonapara for input as I'm not sure. The downside of this is that it feels a bit "magical" and it's hard to figure this out. Probably it will help if we had a view picker on show page or something in following PRs... |
| </FieldContext.Provider> | ||
| ))} | ||
|
|
||
| {renderRelationFields(orderedRelationFields)} |
There was a problem hiding this comment.
Please create a component instead
| fieldMetadataItems.find((item) => item.id === column.fieldMetadataId), | ||
| ) | ||
| .filter((fieldMetadataItem): fieldMetadataItem is FieldMetadataItem => | ||
| Boolean(fieldMetadataItem), |
|
Seems good to me, Not sure about introducing a view switcher but that's something we could offer in the ⌘K menu if needed |
|
@FelixMalfait @lucasbordeau, I'm Sorry! I made a mistake here. I just used the table's view to render those fields and did not consider other views like Kanban. I will look at the codebase again and make the required changes. |
Closes: #9320
Please review thoroughly and be harsh with edge cases.