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.

Sign up for GitHub

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

Jump to bottom

feat: stage instances #5749

Merged
merged 45 commits into from
Jun 14, 2021
Merged

feat: stage instances #5749

merged 45 commits into from
Jun 14, 2021

Conversation

iShibi
Copy link
Contributor

@iShibi iShibi commented Jun 4, 2021

Please describe the changes this PR makes and why it should be merged:

This PR starts the work of adding support for Stage Intances. Since, this PR isn't complete yet and a few PRs related to stage instances are still open in discord-api-docs (3060 & 2998), I'll keep this as a draft for now.

Status and versioning classification:

@ckohen
Copy link
Member

ckohen commented Jun 4, 2021

It looks like there can only be one stage instance at any given time per stage channel. I think implementing this should be done similar to VoiceStates instead. e.g. the manager should be on the guild and the channel should have a getter that returns the current stage instance if any.

@iShibi
Copy link
Contributor Author

iShibi commented Jun 5, 2021

That seems to be a correct way of doing it. I did take some time to decide where the manager should go. Now, if only I can remember why I chose it to be on the stage channel eventually 🐠

@iCrawl iCrawl requested review from SpaceEEC, kyranet and vladfrangu June 5, 2021 06:16
@kyranet
Copy link
Member

kyranet commented Jun 7, 2021

Both PRs were landed, so we're safe to continue forward.

Also, as the tradition goes, this needs a rebase.

@iShibi
Copy link
Contributor Author

iShibi commented Jun 7, 2021

There are still a few things left to be implemented, once they are done I'll mark the PR as ready for review. I have planned to resolve the conflicts by rebasing once everything is done. Also, I'm having exams these days, so I'm sorry if I take some time to respond 😸

vladfrangu
vladfrangu requested changes Jun 8, 2021
View reviewed changes
src/client/actions/StageInstanceUpdate.js Outdated Show resolved Hide resolved
src/managers/StageInstanceManager.js Outdated Show resolved Hide resolved
src/structures/StageChannel.js Outdated Show resolved Hide resolved
src/structures/StageChannel.js Outdated Show resolved Hide resolved
src/structures/StageInstance.js Show resolved Hide resolved
src/structures/StageInstance.js Outdated Show resolved Hide resolved
src/structures/StageInstance.js Outdated Show resolved Hide resolved
@iCrawl iCrawl requested a review from vladfrangu June 9, 2021 13:06
@ImRodry
Copy link
Contributor

ImRodry commented Jun 9, 2021

About the latest commit: shouldn't the names of the variables from .then() functions be a little bit more explicit? I know s stands for stage but some users might not understand it immediately

@iShibi
Copy link
Contributor Author

iShibi commented Jun 9, 2021

I just followed the pattern of existing examples. I don't have any stance (pun intended with no shame 😆) on it. So, I leave the decision on the maintainers.

@ImRodry
Copy link
Contributor

ImRodry commented Jun 9, 2021

Hmm I've never seen variables with names like that before, whereas explicit names are everywhere (take TextChannel as an example). Where did you see those existing examples?

@iShibi
Copy link
Contributor Author

iShibi commented Jun 9, 2021

Guild#delete and Guild#leave, but looks like these were only few of those and I happen to only notice them. I'll make the words more explicit in the example 👍

@iShibi iShibi force-pushed the feat-stage-instance branch from 64aed67 to 0f662d6 Compare June 12, 2021 21:02
@iShibi iShibi marked this pull request as ready for review June 12, 2021 21:28
@iShibi
Copy link
Contributor Author

iShibi commented Jun 12, 2021

Resolved all conflicts. It's ready for review 🎉

SpaceEEC
SpaceEEC requested changes Jun 13, 2021
View reviewed changes
src/managers/StageInstanceManager.js Show resolved Hide resolved
src/structures/StageInstance.js Outdated Show resolved Hide resolved
src/structures/StageInstance.js Outdated Show resolved Hide resolved
@iCrawl
Copy link
Member

iCrawl commented Jun 13, 2021

This needs a rebase 👀

iShibi and others added 17 commits June 14, 2021 00:20
@iShibi
feat(StageInstance): add method to delete the stage instance
9429864
@iShibi
fix(StageInstance): set #deleted of clone to true in delete method
e2ef323
@iShibi
refactor(StageInstance): make channel prop a getter
5137991
@iShibi
feat(GuildAuditLogs): add support for logs related to StageInstance
c11f81a
@iShibi @vladfrangu
refactor: add suggestions from the review
e7a1a68 
Co-authored-by: Vlad Frangu <kingdgrizzle@gmail.com>
@iShibi
refactor(StageInstanceManager): remove redundant add method
ae90203
@iShibi
types: update audit logs typings related to stage instance
8c7bfb9
@iShibi
refactor: revert changes that are out of scope of the PR
e826560
@iShibi
refactor: refactor action handlers for stage instance events
51f6009
@iShibi
feat(StageInstance): add setTopic method and some refactoring
b81b1e8
@iShibi
feat(StageInstance): add creation time related props
0ce8cb9
@iShibi
docs: add basic examples for new methods
f7b2f89
@iShibi
docs: make variables in examples more explicit
4387b6b
@iShibi
docs(AuditLogEntryTarget): revert the unintended change
aa7fff9
@iShibi
refactor: use BaseFetchOptions
0b01c75
@iShibi
docs(StageInstanceManager): document the manager
ad6ee76
@iShibi @SpaceEEC
refactor(StageInstance): use resolve in channel & guild getters
32f2571 
Co-authored-by: SpaceEEC <spaceeec@yahoo.com>
@iShibi iShibi force-pushed the feat-stage-instance branch from 4f5517a to 32f2571 Compare June 13, 2021 18:51
@iShibi
Copy link
Contributor Author

iShibi commented Jun 13, 2021

Done

kyranet
kyranet requested changes Jun 13, 2021
View reviewed changes
src/structures/GuildAuditLogs.js Outdated Show resolved Hide resolved
src/structures/GuildAuditLogs.js Outdated Show resolved Hide resolved
src/managers/StageInstanceManager.js Outdated Show resolved Hide resolved
@iShibi iShibi requested a review from kyranet June 14, 2021 07:36
kyranet
kyranet requested changes Jun 14, 2021
View reviewed changes
Copy link
Member

@kyranet kyranet left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Just two small docs things, and it'll be good to go:

src/managers/StageInstanceManager.js Outdated Show resolved Hide resolved
src/structures/StageInstance.js Outdated Show resolved Hide resolved
@iShibi @kyranet
docs: start comments with uppercase letter
d539ee7 
Co-authored-by: Antonio Román <kyradiscord@gmail.com>
@iCrawl iCrawl merged commit 918921e into discordjs:master Jun 14, 2021
@iShibi iShibi deleted the feat-stage-instance branch June 15, 2021 15:00
@iCrawl iCrawl added this to the Version 13 milestone Jun 27, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vladfrangu vladfrangu vladfrangu approved these changes

@iCrawl iCrawl iCrawl approved these changes

@kyranet kyranet kyranet approved these changes

@SpaceEEC SpaceEEC SpaceEEC approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
Version 13
Development

Successfully merging this pull request may close these issues.
7 participants
@iShibi @ckohen @kyranet @ImRodry @iCrawl @vladfrangu @SpaceEEC