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

Implement object fields and settings new layout #7979

Merged
merged 26 commits into from
Nov 7, 2024
Merged

Conversation

gitstart-app[bot]
Copy link
Contributor

@gitstart-app gitstart-app bot commented Oct 22, 2024

Description

  • This PR has as the base branch the TWNTY-5491 branch, but we also had to include updates from the main branch, and currently, there are conflicts in the TWNTY-5491, that cause errors on typescript in this PR, so, we can update once the conflicts are resolved on the base branch, but the functionality can be reviewed anyway
  • We Implemented a new layout of object details settings and new, the data is auto-saved in Settings tab of object detail
  • There is no indication to the user that data are saved automatically in the design, currently we are disabling the form

Demo\

https://www.loom.com/share/4198c0aa54b5450780a570ceee574838?sid=b4ef0a42-2d41-435f-9f5f-1b16816939f7

Refs

#TWNTY-5491

@gitstart-twenty gitstart-twenty changed the base branch from main to TWNTY-5491 October 22, 2024 23:57
@gitstart-twenty gitstart-twenty changed the base branch from TWNTY-5491 to main October 22, 2024 23:57
Copy link
Contributor

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

PR Summary

This pull request implements a new layout for object details settings, focusing on custom object renaming and synchronization between labels and API names. Here are the key changes:

  • Added a new 'shouldSyncLabelAndName' field to object metadata, enabling label and API name synchronization
  • Implemented auto-saving functionality in the 'Settings' tab of object details
  • Created new components for object fields, indexes, and settings tabs
  • Introduced a SyncObjectLabelAndNameToggle component for managing label and API name synchronization
  • Updated validation schemas to support new fields and synchronization logic
  • Removed the old SettingsObjectEdit component and related stories

The changes align with the requirements outlined in issue #5491, providing more flexibility in object renaming and API name management.

30 file(s) reviewed, 20 comment(s)
Edit PR Review Bot Settings | Greptile

@@ -26,4 +26,5 @@ export const objectMetadataItemSchema = z.object({
namePlural: camelCaseStringSchema,
nameSingular: camelCaseStringSchema,
updatedAt: z.string().datetime(),
shouldSyncLabelAndName: z.boolean(),
}) satisfies z.ZodType<ObjectMetadataItem>;
Copy link
Contributor

Choose a reason for hiding this comment

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

style: consider adding a default value for shouldSyncLabelAndName

Suggested change
}) satisfies z.ZodType<ObjectMetadataItem>;
shouldSyncLabelAndName: z.boolean().default(false),

Comment on lines 8 to 11
const StyledSettingsPageContainer = styled.div<{
width?: number;
removeLeftPadding?: boolean;
}>`
Copy link
Contributor

Choose a reason for hiding this comment

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

style: Consider using a more descriptive name for the removeLeftPadding prop, such as noLeftPadding or leftPaddingDisabled

Suggested change
const StyledSettingsPageContainer = styled.div<{
width?: number;
removeLeftPadding?: boolean;
}>`
const StyledSettingsPageContainer = styled.div<{
width?: number;
noLeftPadding?: boolean;
}>`

Comment on lines 17 to 19
& {
${({ removeLeftPadding }) => removeLeftPadding && `padding-left: 0;`}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

style: Using & selector here is unnecessary. Consider removing it for cleaner CSS

Suggested change
& {
${({ removeLeftPadding }) => removeLeftPadding && `padding-left: 0;`}
}
${({ removeLeftPadding }) => removeLeftPadding && `padding-left: 0;`}

export const ObjectFields = ({ objectMetadataItem }: ObjectFieldsProps) => {
const shouldDisplayAddFieldButton = !objectMetadataItem.isRemote;
Copy link
Contributor

Choose a reason for hiding this comment

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

style: Consider adding a comment explaining the isRemote property and its implications

Suggested change
export const ObjectFields = ({ objectMetadataItem }: ObjectFieldsProps) => {
const shouldDisplayAddFieldButton = !objectMetadataItem.isRemote;
// The isRemote property indicates whether the object is remote or local. If it is remote, the add field button should not be displayed.

Comment on lines 33 to 35
<SettingsObjectFieldTable
objectMetadataItem={objectMetadataItem}
mode="view"
/>
Copy link
Contributor

Choose a reason for hiding this comment

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

logic: Ensure SettingsObjectFieldTable component is updated to handle auto-saving functionality

Suggested change
<SettingsObjectFieldTable
objectMetadataItem={objectMetadataItem}
mode="view"
/>
// Ensure that the SettingsObjectFieldTable component is updated to handle auto-saving functionality.

Comment on lines 14 to 16
const [updatedObjectSlug, setUpdatedObjectSlug] = useRecoilState(
updatedObjectSlugState,
);
Copy link
Contributor

Choose a reason for hiding this comment

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

style: Consider using a more descriptive name for this state, such as 'updatedObjectSlugState'

Suggested change
const [updatedObjectSlug, setUpdatedObjectSlug] = useRecoilState(
updatedObjectSlugState,
);
const [currentUpdatedObjectSlug, setCurrentUpdatedObjectSlug] = useRecoilState(
updatedObjectSlugState,
);

Comment on lines 50 to 51
activeObjectMetadataItem ||
(updatedActiveObjectMetadataItem as ObjectMetadataItem)
Copy link
Contributor

Choose a reason for hiding this comment

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

style: Type assertion might be unnecessary if types are properly inferred

Suggested change
activeObjectMetadataItem ||
(updatedActiveObjectMetadataItem as ObjectMetadataItem)
// Remove type assertion if types are properly inferred
activeObjectMetadataItem || updatedActiveObjectMetadataItem

Comment on lines 73 to 76
const isAdvancedModeEnabled = useRecoilValue(isAdvancedModeEnabledState);

const isUniqueIndexesEnabled = useIsFeatureEnabled(
'IS_UNIQUE_INDEXES_ENABLED',
);
Copy link
Contributor

Choose a reason for hiding this comment

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

style: Consider memoizing this value to prevent unnecessary re-renders

Suggested change
const isAdvancedModeEnabled = useRecoilValue(isAdvancedModeEnabledState);
const isUniqueIndexesEnabled = useIsFeatureEnabled(
'IS_UNIQUE_INDEXES_ENABLED',
);
const isAdvancedModeEnabled = useRecoilValue(isAdvancedModeEnabledState);
const isUniqueIndexesEnabled = useRecoilValue(useIsFeatureEnabled('IS_UNIQUE_INDEXES_ENABLED'));

Comment on lines 100 to 111
const renderActiveTabContent = () => {
switch (activeTabId) {
case 'fields':
return <ObjectFields objectMetadataItem={objectMetadataItem} />;
case 'settings':
return <ObjectSettings objectMetadataItem={objectMetadataItem} />;
case 'indexes':
return <ObjectIndexes objectMetadataItem={objectMetadataItem} />;
default:
return <></>;
}
};
Copy link
Contributor

Choose a reason for hiding this comment

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

style: Consider extracting this switch statement into a separate function or using a mapping object for better maintainability

Suggested change
const renderActiveTabContent = () => {
switch (activeTabId) {
case 'fields':
return <ObjectFields objectMetadataItem={objectMetadataItem} />;
case 'settings':
return <ObjectSettings objectMetadataItem={objectMetadataItem} />;
case 'indexes':
return <ObjectIndexes objectMetadataItem={objectMetadataItem} />;
default:
return <></>;
}
};
const renderActiveTabContent = () => {
const tabContentMap = {
fields: <ObjectFields objectMetadataItem={objectMetadataItem} />,
settings: <ObjectSettings objectMetadataItem={objectMetadataItem} />,
indexes: <ObjectIndexes objectMetadataItem={objectMetadataItem} />,
};
return tabContentMap[activeTabId] || <></>;
};

Copy link
Contributor

Choose a reason for hiding this comment

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

logic: Entire file has been removed. Ensure the object editing functionality has been properly relocated or replaced elsewhere.

@ijreilly
Copy link
Collaborator

Hi @gitstart-twenty thanks for your work, we will wait for https://github.com/twentyhq/twenty/pull/7504/files to be merged before resuming the work here!

@gitstart-twenty
Copy link
Contributor

ok, thank you @ijreilly

@gitstart-app gitstart-app bot force-pushed the TWNTY-5491-new-layout branch from c2e1667 to 6ba2970 Compare October 25, 2024 20:06
Copy link
Member

@Weiko Weiko left a comment

Choose a reason for hiding this comment

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

LGTM!
Btw, does not seem to be related to your change but the sort on the "isUnique" (the key icon in the table header) is broken

},
]}
actionButton={
activeTabId === 'fields' && (
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
activeTabId === 'fields' && (
activeTabId === FIELDS_TAB_ID && (

@Bonapara
Copy link
Member

Fixes

Layout

Margins should match those on other pages. The spacing between the title and the tab switcher should be 32px.

CleanShot.2024-10-29.at.17.39.29.mp4

CleanShot 2024-10-29 at 17 41 08

Card design

Current

CleanShot 2024-10-29 at 17 43 08

Desired

CleanShot 2024-10-29 at 17 43 44

  • Border radius are more round
  • Background is Secondary
  • Height is smaller (there is to much space between the card title and sub-title)
  • Make sure this is a component

Advanced mode mark

CleanShot 2024-10-29 at 17 48 07

Spacing should be 8px with content (-23px?)

Title header

Change the description margin-top to 8px (instead of 12px)

CleanShot 2024-10-29 at 17 50 37

Fields Helper

Current

CleanShot 2024-10-29 at 17 56 15

Desired

CleanShot 2024-10-29 at 17 56 33

URL

Store the tab in the URL with an # e.g. http://localhost:3001/settings/objects/rockets#settings

charlesBochet pushed a commit that referenced this pull request Nov 4, 2024
@Bonapara
Copy link
Member

Bonapara commented Nov 5, 2024

One last thing 

The title padding-top is 24px instead of 32px (like other pages)

CleanShot 2024-11-05 at 10 39 36

And I have no more advanced mode yellow border:

CleanShot 2024-11-05 at 10 41 58

CleanShot 2024-11-05 at 10 43 51

I have a small unexpected horizontal scroll:

CleanShot.2024-11-05.at.10.48.35.mp4

Make description margin-top 4px instead of 8px:

CleanShot 2024-11-05 at 10 49 44

@ijreilly

@ijreilly ijreilly merged commit 7bab65b into main Nov 7, 2024
19 checks passed
@ijreilly ijreilly deleted the TWNTY-5491-new-layout branch November 7, 2024 13:50
Copy link

github-actions bot commented Nov 7, 2024

Fails
🚫

node failed.

Log

�[31mError: �[39m SyntaxError: Unexpected end of JSON input
    at JSON.parse (<anonymous>)
�[90m    at parseJSONFromBytes (node:internal/deps/undici/undici:5584:19)�[39m
�[90m    at successSteps (node:internal/deps/undici/undici:5555:27)�[39m
�[90m    at fullyReadBody (node:internal/deps/undici/undici:1665:9)�[39m
�[90m    at process.processTicksAndRejections (node:internal/process/task_queues:95:5)�[39m
�[90m    at async specConsumeBody (node:internal/deps/undici/undici:5564:7)�[39m
danger-results://tmp/danger-results-9baf61b9.json

Generated by 🚫 dangerJS against 4d828c0

charlesBochet added a commit that referenced this pull request Nov 11, 2024
Follow-up of #7979
Navigation between settings and fields tabs is now reflected in URL. 
<img width="1106" alt="Capture d’écran 2024-11-07 à 18 38 57"
src="https://github.com/user-attachments/assets/24b153ef-9e68-4aa2-8e3a-6bf70834c5ad">

---------

Co-authored-by: gitstart-twenty <[email protected]>
Co-authored-by: gitstart-twenty <[email protected]>
Co-authored-by: Weiko <[email protected]>
Co-authored-by: Charles Bochet <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants