feat: improve the design of the fields widget#17003
Conversation
There was a problem hiding this comment.
Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.
There was a problem hiding this comment.
1 issue found across 2 files
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="packages/twenty-front/src/modules/page-layout/hooks/useTemporaryFieldsConfiguration.ts">
<violation number="1" location="packages/twenty-front/src/modules/page-layout/hooks/useTemporaryFieldsConfiguration.ts:40">
P1: The `position` value uses the index from the original `fieldsToDisplay` array, resulting in non-sequential positions within each section. For example, if LINKS fields are at indices 1 and 3, `generalFields` will have positions 0, 2, 4 (with gaps). Positions should be sequential within each section for proper ordering.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| fieldsToDisplay.forEach((field, index) => { | ||
| const fieldConfig = { | ||
| fieldMetadataId: field.id, | ||
| position: index, |
There was a problem hiding this comment.
P1: The position value uses the index from the original fieldsToDisplay array, resulting in non-sequential positions within each section. For example, if LINKS fields are at indices 1 and 3, generalFields will have positions 0, 2, 4 (with gaps). Positions should be sequential within each section for proper ordering.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/twenty-front/src/modules/page-layout/hooks/useTemporaryFieldsConfiguration.ts, line 40:
<comment>The `position` value uses the index from the original `fieldsToDisplay` array, resulting in non-sequential positions within each section. For example, if LINKS fields are at indices 1 and 3, `generalFields` will have positions 0, 2, 4 (with gaps). Positions should be sequential within each section for proper ordering.</comment>
<file context>
@@ -25,26 +25,56 @@ export const useTemporaryFieldsConfiguration = (
+ fieldsToDisplay.forEach((field, index) => {
+ const fieldConfig = {
+ fieldMetadataId: field.id,
+ position: index,
+ };
+
</file context>
✅ Addressed in d80c2e2
|
Maybe we need to reduce left-right padding instead of completely removing it, but open to feedback. |
|
🚀 Preview Environment Ready! Your preview environment is available at: http://bore.pub:24243 This environment will automatically shut down when the PR is closed or after 5 hours. |
Devessier
left a comment
There was a problem hiding this comment.
Thanks for your work. It looks way better like this.
Indeed, the sections will come from the backend. The changes you made to populate the Other sections seem sufficient to me for the moment.
I wrote two small recommendations. Let's work on them before merging the PR.
| )} | ||
| </StyledHeader> | ||
| {children} | ||
| <AnimatedExpandableContainer isExpanded={isExpanded}> |
There was a problem hiding this comment.
The animation happens when the component mounts. If I remember well, there is an option to ask the library not to animate on mount.
CleanShot.2026-01-08.at.14.18.53.mp4
| {isExpanded ? ( | ||
| <IconChevronUp | ||
| size={theme.icon.size.md} | ||
| stroke={theme.icon.stroke.sm} | ||
| color={theme.font.color.tertiary} | ||
| /> | ||
| ) : ( |
There was a problem hiding this comment.
I feel it would be better to rotate the icon instead of going from chevron up to chevron down. This is a pattern we widely use in the codebase, I guess you can take inspiration from there.
CleanShot.2026-01-08.at.14.20.03.mp4
|
@Devessier done. No animation on first render but during toggles took time to figure out, but got it! |
|
Hey @mabdullahabaid! After you've done the QA of your Pull Request, you can mark it as done here. Thank you! |
1 similar comment
|
Hey @mabdullahabaid! After you've done the QA of your Pull Request, you can mark it as done here. Thank you! |
Closes 2005.
This is how it looks like and I have a feeling that it matches the Figma design. I am not sure if we need to remove more padding as mentioned in the issue since removing it takes it away from the Figma design. Please review and let me know.
Figma design itself:
In terms of data that we receive, I believe it would be handled by Fields Configuration coming from the Backend, so General and Other are decided at that layer, unless I am missing something.