-
Notifications
You must be signed in to change notification settings - Fork 1.3k
feat: table "inline" editing #8754
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
base: main
Are you sure you want to change the base?
Conversation
Build successful! 🎉 |
Build successful! 🎉 |
…nvalid state, click off
Build successful! 🎉 |
Build successful! 🎉 |
Build successful! 🎉 |
Build successful! 🎉 |
Build successful! 🎉 |
Build successful! 🎉 |
Build successful! 🎉 |
let boundaryStartEdge = boundaryDimensions.scroll[AXIS[axis]] + padding; | ||
let boundaryEndEdge = boundarySize + boundaryDimensions.scroll[AXIS[axis]] - padding; | ||
let startEdgeOffset = offset - containerScroll + containerOffsetWithBoundary[axis] - boundaryDimensions[AXIS[axis]]; | ||
let startEdgeOffset = offset - containerScroll + containerOffsetWithBoundary[axis]; |
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.
ignore this change for a bit, might be a bug in our positioning code
…plementation to make generic, fix density, individual cell saving state, use touch detection for showing all the time
**/ | ||
onAction?: () => void | ||
onAction?: () => void, | ||
focusMode?: 'cell' | 'child' |
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.
change we'd need to make if we want to support edit mode on some cells vs others
Build successful! 🎉 |
context={TextContext} | ||
value={{ | ||
slots: { | ||
[DEFAULT_SLOT]: {styles: label({size})}, |
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.
this is just a missing feature we should really have in S2
Build successful! 🎉 |
Build successful! 🎉 |
Build successful! 🎉 |
Build successful! 🎉 |
Build successful! 🎉 |
Build successful! 🎉 |
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.
one thing I noticed when testing is that tabbing through the table will move you to the edit buttons instead of treating the table as a single tab stop like it does for v3 table, not quite sure why. Note that this specifically happens when selection is enabled: https://reactspectrum.blob.core.windows.net/reactspectrum/f8ad4f7b2000787617b3b6097af722ecf8b5679f/storybook-s2/index.html?path=/story/tableview--editable-table&args=selectionMode:multiple
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.
hmmm odd, i'll have a look
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.
o, i see
lastChild is the last checkbox, which is in a row, we focus it without scrolling, which makes the edit button visible
then we let tab naturally move us to the next element, which is the now visible edit button...
I think i've fixed it by applying excludeFromTabOrder since the collection should handle moving to and from it
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.
added tests as well
isPending: isSaving, | ||
isQuiet: !isSaving, | ||
size, | ||
styles: style({ |
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.
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.
probably ok? probably not common that people keyboard focus, then scroll without clicking somewhere
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.
bit unfortunate that the button is sized/the row outlines are made in such a way that the top of the ring sits flush under the top row's border but the bottom of the ring sits on top of the border. Not too bothered by it, we can look into it as a followup
if (!popoverRef.current?.contains(document.activeElement)) { | ||
return false; | ||
} | ||
formRef.current?.requestSubmit(); |
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.
I might just be forgetting prior examples, but I feel like dismissing a popover via clicking outside shouldn't be a confirm/submit action but instead just a cancel action
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.
Design specifically says this should confirm 🤷🏻
Build successful! 🎉 |
Build successful! 🎉 |
Build successful! 🎉 |
Build successful! 🎉 |
## API Changes
@react-spectrum/s2/@react-spectrum/s2:EditableCell+EditableCell {
+ align?: 'start' | 'center' | 'end' = 'start'
+ children: ReactNode
+ className?: ClassNameOrFunction<CellRenderProps>
+ colSpan?: number
+ id?: Key
+ isSaving?: boolean
+ onCancel: () => void
+ onSubmit: () => void
+ renderEditing: () => ReactNode
+ showDivider?: boolean
+ style?: StyleOrFunction<CellRenderProps>
+ textValue?: string
+} |
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.
overall looks pretty good to me, lets get this in for additional browser testing
isPending: isSaving, | ||
isQuiet: !isSaving, | ||
size, | ||
styles: style({ |
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.
bit unfortunate that the button is sized/the row outlines are made in such a way that the top of the ring sits flush under the top row's border but the bottom of the ring sits on top of the border. Not too bothered by it, we can look into it as a followup
Closes RSP Component Milestones (view)
Implements our Editable Cell component for Table's. Note, this is called inline editing by Spectrum at the moment, but it does not correlate to aria inline editing.
This essentially works around the issue of textfields in cells or anything else in cells.
Will be adding more tests
✅ Pull Request Checklist:
📝 Test Instructions:
🧢 Your Project: