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: add support for role icons #6633

Merged
merged 17 commits into from Oct 3, 2021
Merged

Conversation

iShibi
Copy link
Contributor

@iShibi iShibi commented Sep 13, 2021

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

Starts work on role icons:

Status and versioning classification:

  • I know how to update typings and have done so
  • This PR changes the library's interface (methods or parameters added)

Copy link
Contributor

@ImRodry ImRodry left a comment

Choose a reason for hiding this comment

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

Small nitpicks as I know this is still a WIP

src/managers/RoleManager.js Outdated Show resolved Hide resolved
src/managers/RoleManager.js Outdated Show resolved Hide resolved
src/util/Constants.js Outdated Show resolved Hide resolved
src/managers/RoleManager.js Outdated Show resolved Hide resolved
Copy link
Contributor

@NotSugden NotSugden left a comment

Choose a reason for hiding this comment

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

can't you also pass null for the role icon? if so docs should be updated

@Lulalaby
Copy link

can't you also pass null for the role icon? if so docs should be updated

Yes u can

@ImRodry
Copy link
Contributor

ImRodry commented Sep 21, 2021

New upstream PR: discord/discord-api-docs#3847

@iShibi iShibi marked this pull request as ready for review September 22, 2021 04:55
@iShibi
Copy link
Contributor Author

iShibi commented Sep 22, 2021

This is ready for review ✨

@iCrawl iCrawl added this to the Version 13.2 milestone Sep 23, 2021
@Lulalaby
Copy link

hi so i guess it's worth mentioning that, so i'm using this and if I .setIcon() to an animated emoji, absolutely nothing happens, not even an error. But in discord ui you can pick an animated emoji and it'll use the first frame.

yeah, that's a client thing.
But yeah, discord seems like not to throw errors

@ImRodry
Copy link
Contributor

ImRodry commented Sep 29, 2021

hi so i guess it's worth mentioning that, so i'm using this and if I .setIcon() to an animated emoji, absolutely nothing happens, not even an error. But in discord ui you can pick an animated emoji and it'll use the first frame.

Well the fact that the url property on an emoji is a getter and not a function like it is everywhere else makes it hard to add a fix for this. We could just get the png image by calling the CDN endpoint directly because that way we’d always make sure to get the first frame of the emoji

@ghost
Copy link

ghost commented Sep 29, 2021

pardon my stupidity (i was actually setting it to undefined); it works fine. well transparency is lost but yeah

Copy link
Contributor

@advaith1 advaith1 left a comment

Choose a reason for hiding this comment

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

unicode_emoji is now the actual emoji, not a text name

src/managers/RoleManager.js Outdated Show resolved Hide resolved
src/structures/Role.js Outdated Show resolved Hide resolved
src/structures/Role.js Outdated Show resolved Hide resolved
src/structures/Role.js Outdated Show resolved Hide resolved
src/managers/RoleManager.js Show resolved Hide resolved
src/managers/RoleManager.js Show resolved Hide resolved
@iCrawl
Copy link
Member

iCrawl commented Oct 2, 2021

This needs a rebase.

@iCrawl
Copy link
Member

iCrawl commented Oct 3, 2021

This needs a rebase.

@iShibi
Copy link
Contributor Author

iShibi commented Oct 3, 2021

Rebased 😓

@iCrawl iCrawl merged commit 7129965 into discordjs:main Oct 3, 2021
@iShibi iShibi deleted the feat-role-icon branch January 12, 2022 08:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet