Skip to content
This repository has been archived by the owner on Nov 13, 2024. It is now read-only.

Update config.mts to fix links & add Delete post, typing, and ulist #16

Closed
wants to merge 4 commits into from
Closed

Conversation

Finley224
Copy link
Contributor

The docs have broken links that go to the old mower media org and the wrong repos, this commit should fix that.

@Finley224
Copy link
Contributor Author

I know there is another pull request, but this one only fixes the links and does nothing else.

I also moved some stuff around on the side bar, instead of having a list of all the cloudlink stuff, I think it is better to group it into packets (things the server sends), and commands (things the client sends to the server).
@Finley224 Finley224 changed the title Update config.mts to fix links Update config.mts to fix links & add Delete post, typing, and ulist Sep 27, 2024
@Finley224
Copy link
Contributor Author

New addition:
I also moved some stuff around on the sidebar, instead of having a list of all the cloudlink stuff, I think it is better to group it into packets (things the server sends), and commands (things the client sends to the server).

@mybearworld

This comment was marked as off-topic.

@@ -8,4 +8,3 @@
- The general request/response shape
-->

This section is out of date.
Copy link
Contributor

Choose a reason for hiding this comment

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

This section is still out of date

Copy link
Contributor

Choose a reason for hiding this comment

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

...which makes the rest of the added Cloudlink sections a little confusing as those are not out of date

Copy link
Contributor Author

@Finley224 Finley224 Sep 27, 2024

Choose a reason for hiding this comment

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

What about " this section is incomplete"?

Copy link
Contributor

Choose a reason for hiding this comment

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

That would be misleading, information that is on here would contradict itself right now

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added a list of the up to date sections, does that work?

docs/api/cloudlink/packets/ulist.md Outdated Show resolved Hide resolved
```ts [TypeScript]
// Converts the online userlist into a array of strings
// val is the string of users given by the websocket
let onlineUsers: string[] = val.split(";");
Copy link
Contributor

Choose a reason for hiding this comment

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

The JS code works in Typescript as well and is more idiomatic

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, I will merge them into one.

.vitepress/sidebar.mts Outdated Show resolved Hide resolved
Copy link
Contributor

@williamhorning williamhorning left a comment

Choose a reason for hiding this comment

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

in addition to the feedback i have below(?), please run deno fmt and make sure the checks pass

.vitepress/sidebar.mts Show resolved Hide resolved
docs/api/cloudlink/intro.md Show resolved Hide resolved
This section is out of date.
- This page is out of date.

Up to date pages:
Copy link
Contributor

Choose a reason for hiding this comment

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

i don't really like this approach and again, something should update these

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will update this when I get to it, just don't have time at the moment, do you have an idea on how the message should be written @williamhorning ?

Copy link
Contributor

@williamhorning williamhorning Sep 29, 2024

Choose a reason for hiding this comment

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

Actually, it seems as though most the only thing not up to date would be the authpswd page, which we could just add a notice box to. we should probably add a brief introduction to cloudlink on this page though

docs/api/cloudlink/packets/delete_post.md Outdated Show resolved Hide resolved
Comment on lines +3 to +8
This packet is sent when a client sends a typing indicator. These clients will be sent periodically by clients, so you need to setup a expiration system.
### Client typing indicator frequency
- **meo**: 3 seconds
- **Roarer 2**: 3 seconds
- **Meower Svelte**: 4 seconds

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
This packet is sent when a client sends a typing indicator. These clients will be sent periodically by clients, so you need to setup a expiration system.
### Client typing indicator frequency
- **meo**: 3 seconds
- **Roarer 2**: 3 seconds
- **Meower Svelte**: 4 seconds
This packet is sent when a client is currently typing.

again, client behavior shouldn't go here

Copy link
Contributor

Choose a reason for hiding this comment

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

There should probably still be some sort of reference on what "periodically" means though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I disagree on this one, it's a helpful reference to have the typing times in the API docs as there is no standard for it. What if I remove the note about how to implement it but leave a general note of most clients send the packet at least every 4 seconds?

Copy link
Contributor

Choose a reason for hiding this comment

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

the lack of standard is the problem here, but i'm not opposed to saying "send a typing indicator every few seconds" note

Copy link
Member

Choose a reason for hiding this comment

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

Maybe say it expires after 4 seconds

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe say it expires after 4 seconds

Only Meower Svelte seems to use that though

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe say it expires after 4 seconds

Maybe say it expires after 4 seconds

Only Meower Svelte seems to use that though

I like the 4s idea, all the other ones seem to be less than 4s so 4s would work.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

does the 4s thing work for you @williamhorning?

Copy link
Contributor

Choose a reason for hiding this comment

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

@Finley224 either saying "every four seconds" or saying "at an interval less than five seconds" is fine

docs/api/cloudlink/packets/ulist.md Show resolved Hide resolved
@Finley224
Copy link
Contributor Author

in addition to the feedback i have below(?), please run deno fmt and make sure the checks pass

Ok, when I'm back at my computer I will do that.

@Finley224
Copy link
Contributor Author

I will fix the errors with the checks soon.

williamhorning added a commit that referenced this pull request Sep 29, 2024
Co-authored-by: Finley <[email protected]>
williamhorning added a commit that referenced this pull request Sep 29, 2024
Co-authored-by: Finley <[email protected]>
@williamhorning
Copy link
Contributor

merged this in d76ad95

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants