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

Resolve color in embed constructor #2912

Merged
merged 3 commits into from Apr 15, 2019

Conversation

Projects
None yet
5 participants
@TomSputz
Copy link
Contributor

commented Oct 28, 2018

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

Make use of resolveColor utility method in MessageEmbed constructor to mirror setColor

setColor(color) {
this.color = Util.resolveColor(color);
return this;
}

Alternatively, I would say that this resolveColor method is kinda extraneous anyway, and I'd remove the whole thing. It might just not be worth it for backwards compatibility though

Status

  • Code changes have been tested against the Discord API, or there are no code changes
  • I know how to update typings and have done so, or typings don't need updating

Semantic versioning classification:

  • This PR changes the library's interface (methods or parameters added)
    • This PR includes breaking changes (methods removed or renamed, parameters moved or removed)
  • This PR only includes non-code changes, like changes to documentation, README, etc.
@@ -42,7 +42,7 @@ class MessageEmbed {
* The color of this embed
* @type {?number}

This comment has been minimized.

Copy link
@Lewdcario

Lewdcario Oct 28, 2018

Member

This no longer only takes an integer with this change, but rather, ColorResolvable.

  • Edit: My bad, a better suggestion would be to make a new typedef for the data the constructor takes, if others agree.

This comment has been minimized.

Copy link
@TomSputz

TomSputz Oct 29, 2018

Author Contributor

As dim pointed out, the constructor isn't the recommended method of creating embeds, instead it seems the library leans towards using the set methods. The typings already account for the constructor, I'm not sure adding a typedef to it is worth it, as it will only clutter the documentation

@xDimGG

This comment has been minimized.

Copy link
Contributor

commented Oct 28, 2018

Sorry, but this makes no sense to me. The MessageEmbed class is used to represent embeds received from the Discord API and for users to construct one themselves. The purpose for the setup method is to bind the Discord API's data to the class instance. Although users can, they shouldn't be doing new MessageEmbed({ description: 'Hello, world!', color: 0xFFFFFF }); because the MessageEmbed class provides methods to set those fields. If you're planning to use a raw embed object for TextChannel#send you'd be better off doing <TextChannel>.send({ embed: { description: 'Hello, world!', color: 0xFFFFFF } }); rather than <TextChannel>.send(new MessageEmbed({ description: 'Hello, world!', color: 0xFFFFFF /* Or WHITE, in the case of this PR */ }));.

@TomSputz

This comment has been minimized.

Copy link
Contributor Author

commented Oct 29, 2018

It's mostly to maintain consistency, the setColor method should take the same variables as the constructor to stay predictable. As it it now, it isn't so intuitive -

<MessageEmbed>.setColor('WHITE') // All good
<TextChannel>.send(new MessageEmbed({ description: 'Hello, world!', color: 'WHITE' })) // Embed is now invalid

Though I can understand not wanting to use this functionality, your example shows a pretty reasonable use case. If someone does want to use the named colours, I prefer

<TextChannel>.send(new MessageEmbed({
  description: 'Hello, world!',
  color: 'WHITE' 
}))

to

<TextChannel>.send(new MessageEmbed()
  .setDescription('Hello, world!')
  .setColor('WHITE'))

(It'll also be marginally more performant)

@SpaceEEC SpaceEEC merged commit 52bc5b0 into discordjs:master Apr 15, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.