Skip to content

Conversation

@snowystinger
Copy link
Member

Closes

This changes from the popover to a true dialog for touch devices where a keyboard may come into view and mess with the scroll position.
It removes the intermediate ref from the examples and uses the form submission to track updated values.
Adds aria labels and better label examples.
Removes inverted row hover color.

✅ Pull Request Checklist:

  • Included link to corresponding React Spectrum GitHub Issue.
  • Added/updated unit tests and storybook for this change (for new code or code which already has tests).
  • Filled out test instructions.
  • Updated documentation (if it already exists for this component).
  • Looked at the Accessibility Practices for this feature - Aria Practices

📝 Test Instructions:

🧢 Your Project:

@snowystinger snowystinger changed the title fix EditableTable Cells from testing feedback fix: EditableTable Cells from testing feedback Oct 29, 2025
@rspbot
Copy link

rspbot commented Oct 29, 2025

@rspbot
Copy link

rspbot commented Oct 29, 2025

## API Changes

@react-spectrum/s2

/@react-spectrum/s2:EditableCell

 EditableCell {
   align?: 'start' | 'center' | 'end' = 'start'
   children: ReactNode
   colSpan?: number
   id?: Key
   isSaving?: boolean
-  onSubmit: () => void
+  onSubmit: (Record<string, any>) => void
   renderEditing: () => ReactNode
   showDivider?: boolean
   textValue?: string
 }

Copy link
Member

@LFDanLu LFDanLu left a comment

Choose a reason for hiding this comment

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

things look good to me for the most part, just one thing for Android that needs to be changed

};

// Can't differentiate between Dialog click outside dismissal and Escape key dismissal
let isMobile = !useMediaQuery('(any-pointer: fine)');
Copy link
Member

Choose a reason for hiding this comment

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

this doesn't seem to trigger for Android, might need to use touch: '@media not ((hover: hover) and (pointer: fine))', which is what the style macro uses

Comment on lines +1227 to +1242
useEffect(() => {
let dialog = dialogRef.current?.UNSAFE_getDOMNode();
if (isOpen && dialog) {
let handler = (e: KeyboardEvent) => {
if (e.key === 'Escape') {
setIsOpen(false);
e.stopPropagation();
e.preventDefault();
}
};
dialog.addEventListener('keydown', handler);
return () => {
dialog.removeEventListener('keydown', handler);
};
}
}, [isOpen]);
Copy link
Member

Choose a reason for hiding this comment

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

Thats a bit annoying that this needs to be done since dismissible dialogs close on touch start I assume... Feels like a relatively common use case too, wonder if we could support this by default

Copy link
Member Author

Choose a reason for hiding this comment

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

yeah, i was definitely a bit annoyed with this, it's related to these two issues
#1773
#2192

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.

4 participants