Skip to content

Conversation

snowystinger
Copy link
Member

@snowystinger snowystinger commented Oct 7, 2025

Closes

  • Alignment of cancel and confirm buttons is off, safari and FF specific
  • Edit buttons don't appear until you tap on the row
  • Would be nice if the text in the input was auto selected when you started editing
  • If you are on a small screen, the edit box might be too large/small depending on the cell's width. This leads to the edit/cancel buttons being off screen or the user picker being too small and thus you can't see any of the names, just the icons
  • The story code seems quite complicated (lots of state and refs). Maybe we can develop a simpler pattern for the docs
  • After editing a field, I can no longer tab forward out of the table
  • Should update the text in the boundary container story to stop saying that the overlay should match the width of the trigger, it doesn't and shouldn't. Also there should be a visible border

✅ 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:

Comment on lines +315 to +318
// If the active element is NOT tabbable but is contained by an element that IS tabbable (aka the cell), the browser will actually move focus to
// the containing element. We need to special case this so that tab will move focus out of the grid instead of looping between
// focusing the containing cell and back to the non-tabbable child element
if (next && (!next.contains(document.activeElement) || (document.activeElement && !isTabbable(document.activeElement)))) {
Copy link
Member

Choose a reason for hiding this comment

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

Note that I didn't write a test for this because it seems like user.tab() doesn't emulate this browser behavior

@snowystinger snowystinger marked this pull request as ready for review October 7, 2025 20:20
@rspbot
Copy link

rspbot commented Oct 7, 2025

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.

LGTM, just one small question

onCancel={() => {}}
isSaving={item.isSaving[column.id!]}
renderEditing={() => (
<Picker
Copy link
Member

Choose a reason for hiding this comment

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

just a nitpick, but I noticed that opening the picker here on mobile now properly expands the dropdown to grow to fit the available Table space, albiet with a slightly strange "grow" animation that only happens the first time the picker gets opened. However, I was wondering why it doesn't do that for the Async example? I was assuming it would use the width of the table as its bounds per say

Copy link
Member Author

Choose a reason for hiding this comment

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

I also noticed that "grow" animation, I have no idea where it's coming from. There should be a max width on the popover of the table's width,

minWidth: `min(${triggerWidth}px, ${tableWidth}px)`,

Copy link
Member

Choose a reason for hiding this comment

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

For others reviewing the PR, the positioning of the popover being off is expected, the fix is in #8848 (I forgot lol)

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.

3 participants