Skip to content
This repository has been archived by the owner on Jun 29, 2023. It is now read-only.

Fix few docs tickets #448

Closed
wants to merge 23 commits into from
Closed

Fix few docs tickets #448

wants to merge 23 commits into from

Conversation

tapaswenipathak
Copy link

@tapaswenipathak tapaswenipathak commented May 26, 2019

@tapaswenipathak tapaswenipathak changed the title [Minor] Document 'D' direct channel Document 'D' direct channel May 26, 2019
@hanzei hanzei requested a review from jwilander May 27, 2019 08:57
@tapaswenipathak tapaswenipathak changed the title Document 'D' direct channel Fix few docs tickets May 30, 2019
@jwilander
Copy link
Member

Thanks @tapaswenipathak ! Sorry for the slow response, just got back from vacation. I'll take a look shortly

Copy link
Member

@jwilander jwilander left a comment

Choose a reason for hiding this comment

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

Sorry for the late review, lost this one under the radar

v4/source/channels.yaml Outdated Show resolved Hide resolved
v4/source/introduction.yaml Show resolved Hide resolved
v4/source/introduction.yaml Outdated Show resolved Hide resolved
@hanzei hanzei requested a review from jwilander June 12, 2019 18:12
@jasonblais
Copy link
Contributor

@jwilander Quick reminder to review updates when you have the chance :)

@jwilander
Copy link
Member

Thanks :) I'll look soon

v4/source/introduction.yaml Outdated Show resolved Hide resolved
@hanzei hanzei requested a review from jwilander June 23, 2019 17:05
v4/source/introduction.yaml Outdated Show resolved Hide resolved
v4/source/introduction.yaml Outdated Show resolved Hide resolved
@hanzei hanzei requested a review from jwilander June 25, 2019 19:37
Copy link
Member

@jwilander jwilander left a comment

Choose a reason for hiding this comment

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

Can you make sure the payloads for the WS events are accurate? There seem to be some other ones that are wrong other than the ones I commented on

v4/source/introduction.yaml Outdated Show resolved Hide resolved
v4/source/introduction.yaml Outdated Show resolved Hide resolved
@hanzei hanzei requested a review from jwilander July 2, 2019 21:43
v4/source/introduction.yaml Outdated Show resolved Hide resolved
@mattermod
Copy link
Contributor

This issue has been automatically labelled "stale" because it hasn't had recent activity.
A core team member will check in on the status of the PR to help with questions.
Thank you for your contribution!

/cc @jasonblais @hanzei

@hanzei hanzei requested a review from jwilander July 23, 2019 09:01
v4/source/introduction.yaml Outdated Show resolved Hide resolved
v4/source/introduction.yaml Outdated Show resolved Hide resolved
v4/source/introduction.yaml Outdated Show resolved Hide resolved
v4/source/introduction.yaml Outdated Show resolved Hide resolved
v4/source/introduction.yaml Outdated Show resolved Hide resolved
v4/source/introduction.yaml Outdated Show resolved Hide resolved
@tapaswenipathak
Copy link
Author

@jwilander Does the doc look ok? If this doc looks ok, I will create a PR adding remaining things.

v4/source/introduction.yaml Outdated Show resolved Hide resolved
v4/source/introduction.yaml Outdated Show resolved Hide resolved
v4/source/introduction.yaml Outdated Show resolved Hide resolved
v4/source/introduction.yaml Outdated Show resolved Hide resolved
v4/source/introduction.yaml Outdated Show resolved Hide resolved
v4/source/introduction.yaml Outdated Show resolved Hide resolved
v4/source/introduction.yaml Outdated Show resolved Hide resolved
'IsLicensed': '<true/false>'
}
}
}
Copy link
Member

Choose a reason for hiding this comment

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

This one should have a seq_id and broadcast right?

Copy link
Author

Choose a reason for hiding this comment

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

v4/source/introduction.yaml Outdated Show resolved Hide resolved
tapaswenipathak and others added 2 commits July 31, 2019 18:08
Co-Authored-By: Joram Wilander <[email protected]>
Co-Authored-By: Joram Wilander <[email protected]>
@cpanato
Copy link
Contributor

cpanato commented Jul 31, 2019

@tapaswenipathak update your PR with the latest master

@tapaswenipathak
Copy link
Author

@cpanato I added the little merge commit from upstream - a665401. CI should be fine now.

@mattermod
Copy link
Contributor

This issue has been automatically labelled "stale" because it hasn't had recent activity.
A core team member will check in on the status of the PR to help with questions.
Thank you for your contribution!

/cc @jasonblais @hanzei

@hanzei
Copy link
Contributor

hanzei commented Aug 31, 2019

@jwilander gentle reminder to give this another look

'update_at': '<timestamp>',
'delete_at': '<timestamp>',
'team_id': '<team_id>',
'type': 'O',
Copy link
Member

Choose a reason for hiding this comment

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

This can be 'P', 'D' or 'G' as well

'type': 'O',
'display_name':'<channel_id>',
'name': '<channel_name>',
'header': 'header',
Copy link
Member

Choose a reason for hiding this comment

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

This should be a blank string right?

'header': 'header',
'purpose': '',
'last_post_at': '<timestamp>',
'total_msg_count': '<total_msg_count>',
Copy link
Member

Choose a reason for hiding this comment

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

There shouldn't be quotes here as this is a number. Same with the timestamp fields

Copy link
Member

Choose a reason for hiding this comment

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

There's a lot of other cases throughout the changes with this issue

@wiersgallak
Copy link

Hi @tapaswenipathak. Looks like this ticket has not made much progress and there may be outstanding issues. After following up with the team, we think it may be best to close the PR at this time. We recommend potentially spending some additional time with our code base and proposing smaller PRs for changes in the future. We appreciate the time you put into this project.

@tapaswenipathak
Copy link
Author

tapaswenipathak commented Sep 4, 2019 via email

@wiersgallak
Copy link

Closing pull request.

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.

Help Wanted: websocket event datastructure is not documented 'D' channel type is not documented
7 participants