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

feat(Widget): wrapper for widget.json #5619

Merged
merged 30 commits into from
Jun 15, 2021
Merged

feat(Widget): wrapper for widget.json #5619

merged 30 commits into from
Jun 15, 2021

Conversation

Thumuss
Copy link
Contributor

@Thumuss Thumuss commented May 13, 2021

Please describe the changes this PR makes and why it should be merged:
Okay, I'm remaking a new pr because my last was really terrible + i've deleted the original (#5030)

My goal is to add Widget in discord.js
Widget is a part of Discord and it's not fully featured

Status and versioning classification:

  • I know how to update typings and have done so, or typings don't need updating
  • Code changes have been tested against the Discord API, or there are no code changes
  • This PR changes the library's interface (methods or parameters added)

Thanks to @ckohen @advaith1 @SpaceEEC @almostSouji @vladfrangu @NotSugden @kyranet @Mesteery for helping me in the last pull request

@Thumuss Thumuss mentioned this pull request May 13, 2021
5 tasks
typings/index.d.ts Outdated Show resolved Hide resolved
src/structures/Widget.js Outdated Show resolved Hide resolved
src/client/Client.js Outdated Show resolved Hide resolved
src/structures/Widget.js Outdated Show resolved Hide resolved
src/structures/Widget.js Outdated Show resolved Hide resolved
src/structures/Widget.js Outdated Show resolved Hide resolved
src/structures/Widget.js Outdated Show resolved Hide resolved
typings/index.d.ts Outdated Show resolved Hide resolved
typings/index.d.ts Outdated Show resolved Hide resolved
@Thumuss
Copy link
Contributor Author

Thumuss commented May 13, 2021

It's a mess in the logs but I've added in client an Promise.reject in Client#fetchWidget + clean up the if condition in the Widget class

src/client/Client.js Outdated Show resolved Hide resolved
src/client/Client.js Outdated Show resolved Hide resolved
src/structures/Widget.js Outdated Show resolved Hide resolved
src/structures/Widget.js Outdated Show resolved Hide resolved
src/structures/Widget.js Outdated Show resolved Hide resolved
@Thumuss
Copy link
Contributor Author

Thumuss commented May 13, 2021

I have add a new function, exactly the same as the Client#fetchWidget but in the Widget to update the widget
And I fixed the jsdoc like you said vladfrangu

@kyranet kyranet changed the title add(Widget): wrapper for widget.json feat(Widget): wrapper for widget.json May 13, 2021
@Thumuss Thumuss requested a review from vladfrangu May 13, 2021 21:20
typings/index.d.ts Outdated Show resolved Hide resolved
typings/index.d.ts Outdated Show resolved Hide resolved
@Thumuss
Copy link
Contributor Author

Thumuss commented May 27, 2021

I have done a test but that change anything I guess ? I've just link my local to my github project (3ead9e6)

kyranet
kyranet previously requested changes Jun 7, 2021
typings/index.d.ts Show resolved Hide resolved
src/structures/WidgetMember.js Outdated Show resolved Hide resolved
src/structures/WidgetMember.js Show resolved Hide resolved
src/structures/WidgetMember.js Show resolved Hide resolved
@Thumuss Thumuss requested a review from kyranet June 9, 2021 01:32
@iCrawl iCrawl dismissed stale reviews from kyranet, SpaceEEC, and vladfrangu June 9, 2021 07:46

Stale

Copy link
Member

@SpaceEEC SpaceEEC left a comment

Choose a reason for hiding this comment

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

Otherwise LGTM

src/index.js Show resolved Hide resolved
Co-authored-by: SpaceEEC <[email protected]>
Copy link
Member

@kyranet kyranet left a comment

Choose a reason for hiding this comment

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

Just some formatting things for consistency, otherwise LGTM

src/structures/WidgetMember.js Outdated Show resolved Hide resolved
src/structures/WidgetMember.js Outdated Show resolved Hide resolved
src/structures/WidgetMember.js Outdated Show resolved Hide resolved
src/structures/WidgetMember.js Outdated Show resolved Hide resolved
src/structures/WidgetMember.js Outdated Show resolved Hide resolved
src/structures/WidgetMember.js Outdated Show resolved Hide resolved
src/structures/WidgetMember.js Outdated Show resolved Hide resolved
src/structures/WidgetMember.js Outdated Show resolved Hide resolved
src/structures/Widget.js Outdated Show resolved Hide resolved
src/structures/Widget.js Outdated Show resolved Hide resolved
@kyranet kyranet requested a review from iCrawl June 9, 2021 12:58
src/structures/Widget.js Show resolved Hide resolved
src/structures/WidgetMember.js Show resolved Hide resolved
src/structures/WidgetMember.js Show resolved Hide resolved
src/structures/WidgetMember.js Outdated Show resolved Hide resolved
src/structures/WidgetMember.js Outdated Show resolved Hide resolved
src/structures/WidgetMember.js Outdated Show resolved Hide resolved
src/structures/WidgetMember.js Outdated Show resolved Hide resolved
src/structures/WidgetMember.js Outdated Show resolved Hide resolved
src/structures/WidgetMember.js Outdated Show resolved Hide resolved
src/structures/WidgetMember.js Outdated Show resolved Hide resolved
@Thumuss
Copy link
Contributor Author

Thumuss commented Jun 9, 2021

Something I didn't see was the property fetchWidget in Guild (Guild#fetchWidget).
It's actually an other api request. I'm thinking about rename Guild#fetchWidget to Guild#fetchWidgetOption and re use the Client#fetchWidget for Guild#fetchWidget
It is a good idea or do I need to do something else ?

src/structures/WidgetMember.js Outdated Show resolved Hide resolved
src/structures/Widget.js Outdated Show resolved Hide resolved
@Thumuss
Copy link
Contributor Author

Thumuss commented Jun 10, 2021

Before push it, do we need add a Guild#fetchWidget like I said upper ?

src/structures/Widget.js Outdated Show resolved Hide resolved
src/structures/WidgetMember.js Outdated Show resolved Hide resolved
typings/index.d.ts Show resolved Hide resolved
@iCrawl iCrawl merged commit 038ee99 into discordjs:master Jun 15, 2021
@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
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants