-
Notifications
You must be signed in to change notification settings - Fork 3.6k
[Permissions] Force open title input for role label when empty #12710
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
Conversation
Weiko
commented
Jun 18, 2025
- Fix empty title in breadcrumb
- Enforce role label input open if empty
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.
PR Summary
Introduces Recoil-based state management for title inputs with enforced open state for empty role labels, improving UX and consistency across the application.
- Added new
titleInputComponentState
with instance isolation using Recoil, replacing local React state inTitleInput.tsx
- Created
SettingsRoleLabelContainerEffect
to enforce open state for empty role labels, following side-effect pattern - Standardized empty role label placeholder text to 'Untitled Role' across components
- Added unique
instanceId
prop requirement forTitleInput
components to support isolated state management
10 files reviewed, 2 comments
Edit PR Review Bot Settings | Greptile
...twenty-front/src/modules/settings/roles/role/components/SettingsRoleLabelContainerEffect.tsx
Show resolved
Hide resolved
isTitleInputOpen, | ||
]); | ||
|
||
return <></>; |
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.
style: Empty fragment not needed since this is an effect component, use null instead
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.
Nice job ! About to test locally
); | ||
|
||
useEffect(() => { | ||
if (settingsDraftRole.label === '' && !isTitleInputOpen) { |
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.
Subjectives thoughts/questions: Could the user insert an empty string ? From my understanding this assertion aims to compare if the current value is the default one ?
Would it makes sense for you to do such a thing ?
// packages/twenty-front/src/modules/settings/roles/states/settingsDraftRoleFamilyState.ts
export const settingsDraftRoleFamilyStateDefaultValue ={
id: '',
label: '',
description: '',
icon: '',
canDestroyAllObjectRecords: false,
canReadAllObjectRecords: false,
canSoftDeleteAllObjectRecords: false,
canUpdateAllObjectRecords: false,
canUpdateAllSettings: false,
isEditable: false,
workspaceMembers: [],
settingPermissions: [],
objectPermissions: [],
} as const
export const settingsDraftRoleFamilyState = createFamilyState<Role, string>({
key: 'settingsDraftRoleFamilyState',
defaultValue: settingsDraftRoleFamilyStateDefaultValue
});
// packages/twenty-front/src/modules/settings/roles/role/components/SettingsRole.tsx
if (settingsDraftRole.label === settingsDraftRoleFamilyStateDefaultValue.label && !isTitleInputOpen) {
@@ -288,14 +299,19 @@ export const SettingsRole = ({ roleId, isCreateMode }: SettingsRoleProps) => { | |||
href: getSettingsPath(SettingsPath.Roles), | |||
}, | |||
{ | |||
children: settingsDraftRole.label, | |||
children: | |||
isDefined(settingsDraftRole.label) && |
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.
Nitpick: Unless I'm mistaken could use isNonEmptyString
RemarksWhile testing locally I encountered two "weird' behaviors. The role label enforcement prevents:
Screen.Recording.2025-06-19.at.15.26.29.mov
Screen.Recording.2025-06-19.at.15.29.32.mov |
…yhq#12710) - Fix empty title in breadcrumb - Enforce role label input open if empty