Skip to content
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

docs(tree): updates and fixes #23455

Merged
merged 5 commits into from
Jan 8, 2025

Conversation

yann-achard-MS
Copy link
Contributor

Description

Updates and fixes to the existing docs.

Breaking Changes

None

@github-actions github-actions bot added area: website base: main PRs targeted against main branch labels Jan 4, 2025
@yann-achard-MS yann-achard-MS marked this pull request as ready for review January 6, 2025 16:29
@Copilot Copilot bot review requested due to automatic review settings January 6, 2025 16:29

Choose a reason for hiding this comment

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

Copilot wasn't able to review any files in this pull request.

Files not reviewed (6)
  • docs/docs/data-structures/tree/events.mdx: Language not supported
  • docs/docs/data-structures/tree/nodes.mdx: Language not supported
  • docs/docs/data-structures/tree/reading-and-editing.mdx: Language not supported
  • docs/docs/data-structures/tree/schema-definition.mdx: Language not supported
  • docs/docs/data-structures/tree/transactions.mdx: Language not supported
  • docs/docs/data-structures/tree/undo-redo.mdx: Language not supported
@@ -3,7 +3,7 @@ title: Reading and Editing
sidebar_position: 4
---

The `TreeView` object and its children provide methods that enable your code to add nodes to the tree, remove nodes, and move nodes within the tree. You can also set and read the values of leaf nodes. The APIs have been designed to match as much as possible the syntax of TypeScript primitives, objects, maps, and arrays; although some editing APIs are different for the sake of making the merge semantics clearer.
The `TreeView` object and its children expose properties and methods that allows applications read and edit the tree. The APIs have been designed to match as much as possible the syntax of TypeScript primitives, objects, maps, and arrays; although some editing APIs are different for the sake of making the merge semantics clearer.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
The `TreeView` object and its children expose properties and methods that allows applications read and edit the tree. The APIs have been designed to match as much as possible the syntax of TypeScript primitives, objects, maps, and arrays; although some editing APIs are different for the sake of making the merge semantics clearer.
The `TreeView` object and its children expose properties and methods that allow applications read and edit the tree.
The APIs have been designed to match as much as possible the syntax of TypeScript primitives, objects, maps, and arrays (although some editing APIs are different for the sake of making the merge semantics clearer).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@@ -52,6 +52,10 @@ We show how to add this note to an array of notes in the tree in [Array node API
### Editing Object Properties

To update the property on an object node, you assign a new node or value to it with the assignment operator (`=`).
See [here](https://github.com/microsoft/FluidFramework/blob/main/packages/dds/tree/docs/user-facing/object-merge-semantics.md) for details on the merge semantics of these edits.

Note that if the new value is a node (as opposed to a primitive), then its [status](./nodes.mdx#treestatus) must be `TreeStatus.New`,
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this important enough to list before the examples? I think allowing readers to digest the code first, and the warning second, might make this less of a rabbit hole.

Copy link
Contributor

Choose a reason for hiding this comment

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

+1, but this section also requires some deduping with the quick start page so I feel like it's prob fine to make these changes as reminders of info we need to include and rearrange later

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Moved it down


`rootChanged` fires when the root field (the field that contains the root node) changes.
`rootChanged` fires when the contents root field (the field that contains the root node) change.
Copy link
Contributor

Choose a reason for hiding this comment

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

is it supposed to be this?

Suggested change
`rootChanged` fires when the contents root field (the field that contains the root node) change.
`rootChanged` fires when the root field contents (the field that contains the root node) change.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Almost :)
I think "the contents of the root field" make the parenthetical that follow flow a bit better

That is, if a new root node is assigned or the schema changes.
This will not fire when the node itself changes.

`changed` fires whenever a change is applied outside of a transaction or when a transaction is committed.
`commitApplied` fires whenever a change is applied outside of a transaction or when a transaction is committed.
Copy link
Contributor

Choose a reason for hiding this comment

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

commitApplied is being deprecated in favor of changed because commitApplied doesn't fire for remote changes

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ha thanks, I was confused because the current doc still refers to "commitApplied" at the top.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, but it's an alpha API at the moment. Based on this morning's conversation, the consensus seemed to be that we wouldn't add FF-site docs for alpha APIs.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah ok yeah, that makes sense. Hopefully we can make that not alpha soon cause it's very confusing :/

## Node-Level Events

`SharedTree` supports two node-level events: `nodeChanged` and `treeChanged`.
These can be subscribed to using the `Tree.on` method, which has the following signature:
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should add an example of how to subscribe to the tree level events as well in the section above

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@@ -10,13 +10,16 @@ See [node types](./node-types.mdx) for details on the types of nodes that can be

Below are some utilities provided to make working with nodes easier.

### Node Information
### `Tree.key`
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm thinking it'd be better to just have links to the api docs so we're not duplicating any info. It'd be even better if there's a way to embed the docs in pages. @Josmithr do you know if this is a thing?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I'm not sure what the best middle ground is between reducing duplication and facilitating using discovery/usage, but embedding or linking to the API docs sounds good.
I don't think we should do that as part of this PR though.

@@ -52,6 +52,10 @@ We show how to add this note to an array of notes in the tree in [Array node API
### Editing Object Properties

To update the property on an object node, you assign a new node or value to it with the assignment operator (`=`).
See [here](https://github.com/microsoft/FluidFramework/blob/main/packages/dds/tree/docs/user-facing/object-merge-semantics.md) for details on the merge semantics of these edits.

Note that if the new value is a node (as opposed to a primitive), then its [status](./nodes.mdx#treestatus) must be `TreeStatus.New`,
Copy link
Contributor

Choose a reason for hiding this comment

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

+1, but this section also requires some deduping with the quick start page so I feel like it's prob fine to make these changes as reminders of info we need to include and rearrange later


```typescript
keys(): IterableIterator<string>
```

Returns an Iterator that contains the keys in the map node. The keys are iterated in the order that they were added.
Returns an Iterator that contains the keys in the map node.
We do not make guarantees about the ordering of the keys.
Copy link
Contributor

Choose a reason for hiding this comment

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

I think specific info like this should be kept on the API docs and we should do more linking

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, agreed. I mostly wanted to stop telling lies, so I'm happy to just remove the lie and not mention the truth here (the API does say "If your usage scenario depends on consistent ordering, you will need to sort these yourself").

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, I like that approach

@@ -5,9 +5,16 @@ sidebar_position: 5

## Whole-Tree Events

The `TreeView` object exposes 2 events that communicate changes that affect the whole tree.
The `TreeView` object exposes 2 events that communicate changes that affect the whole tree. For example:
Copy link
Contributor

Choose a reason for hiding this comment

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

I think the section below flows a bit better, in how it introduces the events and then says "here's how you subscribe to them"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

Copy link
Contributor

@jenn-le jenn-le left a comment

Choose a reason for hiding this comment

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

looks good to me, just had the one minor comment

Copy link
Contributor

github-actions bot commented Jan 8, 2025

🔗 No broken links found! ✅

Your attention to detail is admirable.

linkcheck output


> [email protected] ci:check-links /home/runner/work/FluidFramework/FluidFramework/docs
> start-server-and-test "npm run serve -- --no-open" 3000 check-links

1: starting server using command "npm run serve -- --no-open"
and when url "[ 'http://127.0.0.1:3000' ]" is responding with HTTP status code 200
running tests using command "npm run check-links"


> [email protected] serve
> docusaurus serve --no-open

[SUCCESS] Serving "build" directory at: http://localhost:3000/

> [email protected] check-links
> linkcheck http://localhost:3000 --skip-file skipped-urls.txt

Crawling...

Stats:
  170508 links
    1603 destination URLs
    1842 URLs ignored
       0 warnings
       0 errors


@yann-achard-MS yann-achard-MS merged commit ba65d67 into microsoft:main Jan 8, 2025
29 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: website base: main PRs targeted against main branch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants