-
Notifications
You must be signed in to change notification settings - Fork 532
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
tree2: add sequence mutators #17523
tree2: add sequence mutators #17523
Conversation
experimental/dds/tree2/src/feature-libraries/editable-tree-2/editableTreeTypes.ts
Outdated
Show resolved
Hide resolved
experimental/dds/tree2/src/feature-libraries/editable-tree-2/editableTreeTypes.ts
Outdated
Show resolved
Hide resolved
experimental/dds/tree2/src/feature-libraries/editable-tree-2/editableTreeTypes.ts
Outdated
Show resolved
Hide resolved
experimental/dds/tree2/src/feature-libraries/editable-tree-2/editableTreeTypes.ts
Outdated
Show resolved
Hide resolved
experimental/dds/tree2/src/feature-libraries/editable-tree-2/editableTreeTypes.ts
Outdated
Show resolved
Hide resolved
experimental/dds/tree2/src/feature-libraries/editable-tree-2/lazyField.ts
Outdated
Show resolved
Hide resolved
* @param index - The index at which to insert `value`. | ||
* @param value - The content to insert. | ||
*/ | ||
insertAt(index: number, value: FlexibleNodeContent<TTypes>[]): void; |
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.
What are the merge semantics here? I think it would be valuable to call them out (here and in other methods as appropriate). Are we inserting after the previous item? Before the following item?
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.
Neither; all methods shipped as part of MVP are index-centric, so they will result in insertion at the index specified unless concurrent changes push them forward (first-write-wins). When we introduce anchored changes, we can add those docs, but for now it's intuitive enough to omit (I validated this opinion with Noah/Alex).
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.
Okay. That's probably fair. Still might be worth noting that. If I were looking at this API (as I was as a reviewer), that is one of the first questions I would be asking.
experimental/dds/tree2/src/feature-libraries/editable-tree-2/editableTreeTypes.ts
Outdated
Show resolved
Hide resolved
experimental/dds/tree2/src/feature-libraries/editable-tree-2/lazyField.ts
Show resolved
Hide resolved
experimental/dds/tree2/src/feature-libraries/editable-tree-2/lazyField.ts
Outdated
Show resolved
Hide resolved
experimental/dds/tree2/src/feature-libraries/typed-schema/typedTreeSchema.ts
Outdated
Show resolved
Hide resolved
experimental/dds/tree2/src/feature-libraries/editable-tree-2/editableTreeTypes.ts
Outdated
Show resolved
Hide resolved
experimental/dds/tree2/src/feature-libraries/editable-tree-2/editableTreeTypes.ts
Outdated
Show resolved
Hide resolved
experimental/dds/tree2/src/feature-libraries/editable-tree-2/editableTreeTypes.ts
Show resolved
Hide resolved
experimental/dds/tree2/src/feature-libraries/editable-tree-2/editableTreeTypes.ts
Show resolved
Hide resolved
experimental/dds/tree2/src/feature-libraries/editable-tree-2/editableTreeTypes.ts
Outdated
Show resolved
Hide resolved
Is the intention to add unit tests for the new behaviors in this PR, or separately? |
* @param sourceEnd - The ending index of the range to move (exclusive) | ||
* @throws Throws if any of the input indices are invalid. | ||
* @remarks | ||
* All indices are relative to the sequence excluding the nodes being moved. |
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 doesn't seem user friendly IMO. Unless I'm missing something. The consumer is required to do the necessary offset math to figure out what index
would be if the selected items were not in the list. Couldn't we do that math for them?
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.
Yea, we are planning to do that at a lower level (i.e. inside of the sequence editor code). If that doesn't land by MVP, we can do the math at this higher layer, I suppose.
⯅ @fluid-example/bundle-size-tests: +956 Bytes
Baseline commit: 14d4581 |
@taylorsw04 - I'm in the market for some sequence mutators to write test cases... hope you don't mind my merging. :-) |
This PR implements sequence mutation methods. Tests will be added in a follow-up PR when #17475 is complete.