Skip to content
This repository was archived by the owner on Apr 27, 2024. It is now read-only.

add GuildMember#displayColour#321

Closed
Stitch07 wants to merge 5 commits intodirigeants:masterfrom
Stitch07:master
Closed

add GuildMember#displayColour#321
Stitch07 wants to merge 5 commits intodirigeants:masterfrom
Stitch07:master

Conversation

@Stitch07
Copy link
Copy Markdown

@Stitch07 Stitch07 commented Jul 27, 2020

Adds a displayColour property to GuildMember

alexthemaster
alexthemaster approved these changes Jul 27, 2020
Copy link
Copy Markdown

@Quantumlyy Quantumlyy left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Copy Markdown

@cfanoulis cfanoulis left a comment

Choose a reason for hiding this comment

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

a nit

if (!highestRole || role.position < highestRole.position) continue;
if (role.color) highestRole = role;
}
return highestRole ? highestRole.color : 0;
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

this should return the hexadecimal

Suggested change
return highestRole ? highestRole.color : 0;
return highestRole ? highestRole.color : 0x000000;

Copy link
Copy Markdown

@Rexogamer Rexogamer left a comment

Choose a reason for hiding this comment

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

lgtm, doc nit

Comment thread src/lib/caching/structures/guilds/GuildMember.ts Outdated
Co-authored-by: Ed L <beartechtalks@gmail.com>
public get displayColor(): number {
let highestRole = null;
for (const role of this.roles.values()) {
if (!highestRole || role.position < highestRole.position) continue;
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Am I misreading this or is this totally bugged? Above it is defined as null then in every iteration this is just going to simply skip the loop. It's null so if (!null ) will always pass and continue to next loop meaning it will never really get the color

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Second bug on this is that it does not take into account when the role positions are equal. Yes that is possible

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

May I ask when can role positions be equal?

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

@Stitch07 Stitch07 changed the title add GuildMember#displayColor add GuildMember#displayColour Jul 27, 2020
@Stitch07
Copy link
Copy Markdown
Author

@Skillz4Killz pushed fixes.

}
highestRole = role;
}
return highestRole ? highestRole.color : 0;
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Suggested change
return highestRole ? highestRole.color : 0;
return highestRole?.color || 0;

* The displayed colour of the member.
* @since 0.0.4
*/
public get displayColour(): number {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
public get displayColour(): number {
public get displayColor(): number {

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

False

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

This is actually more consistent with the rest of the lib. For example, highestRole.color as opposed to highestRole.colour

}

/**
* The displayed colour of the member.
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
* The displayed colour of the member.
* The displayed color of the member.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Fake

@Stitch07 Stitch07 closed this Aug 6, 2020
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.

9 participants