Skip to content

Conversation

paula-stacho
Copy link
Collaborator

@paula-stacho paula-stacho commented Sep 24, 2025

External Links

  • 🎟️ MIG-XXXX
  • 🎨 Figma

Description

If the field is editable, and onFieldNameChange is provided, the field becomes editable on double click.
The style is minimalistic, in line with the inline inputs in Compass CRUD.

Note: I did try LG text input but the customization needed goes beyond what the component supports

Notes for Reviewers

πŸ“Έ Screenshots/Screencasts

Screen.Recording.2025-09-26.at.13.10.33.mov

Before

After

@paula-stacho paula-stacho self-assigned this Sep 24, 2025
Base automatically changed from refactor/COMPASS-9877-add-editable-node-handlers-and-interactions to main September 25, 2025 16:43
Copy link
Member

@Anemy Anemy left a comment

Choose a reason for hiding this comment

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

Looking good, haven't tried it out a yet. Left a couple small comments (not blockers).

@paula-stacho paula-stacho marked this pull request as ready for review September 26, 2025 10:41
@paula-stacho
Copy link
Collaborator Author

@Anemy switched to input and updated styles so that there is something to click on even if the name is very short or empty (which we want to prevent but I don't think we can do it here since the error handling is planned in compass)

@Anemy Anemy requested review from lchans and adrianhhong October 2, 2025 09:53

export const FieldNameContent = ({ name, isEditable, onChange }: FieldNameProps) => {
const [isEditing, setIsEditing] = useState(false);
const [value, setValue] = useState(name);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Would name ever change externally? Would we need a useEffect to re-hydrate this state?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It might actually, we have a sidebar where they can edit too


useEffect(() => {
if (isEditing) {
setTimeout(() => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

What's this setTimeout used for? Do we need this?

setValue(e.target.value);
}}
onBlur={handleSubmit}
onKeyDown={e => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we move this and the onDoubleClick into its own function for better readability, like the other components in this repo?

onBlur?: () => void;
}

export const FieldNameContent = ({ name, isEditable, onChange }: FieldNameProps) => {
Copy link
Collaborator

@lchans lchans Oct 3, 2025

Choose a reason for hiding this comment

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

Would it be better to have two seperate components, one which is editable and the other (the current flow) which is not? At the moment we're passing in quite a few (optional) props which require a conditional check, especially in that useEffect. I could see someone accidentally adding in a change which re-renders this whole component too, rather than just the edited component and vice-versa

onBlur={handleSubmit}
onKeyDown={e => {
if (e.key === 'Enter') handleSubmit();
if (e.key === 'Escape') setIsEditing(false);
Copy link
Collaborator

Choose a reason for hiding this comment

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

To me it's more expected behaviour to reset the field name to it's original value on Escape as opposed to committing the changes.

@adrianhhong adrianhhong self-requested a review October 3, 2025 01:58
Copy link
Collaborator

@adrianhhong adrianhhong left a comment

Choose a reason for hiding this comment

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

Small UX detail, otherwise implementation LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants