-
Notifications
You must be signed in to change notification settings - Fork 49
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
Feature/add colour to list #149
Feature/add colour to list #149
Conversation
added in colour for some list text items need to add more for the rest consider renaming items in style colours
Hey @anontyro as mention in the Discord server a couple days ago, there was one other contributor working along the same lines as you, so there are conflicts in your branch. Please resolve these and then I'll review your changes. Thanks! |
Maybe you can move the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I left some notes for the current change set, but in addition conflicts will need resolving as well. Please sync your branch with the current state of the master branch, resolve conflicts, and address the requested changes so that we can move forward with integrating your work into the codebase. Thanks!
I also like the idea from @RazCrimson about incorporating the Styles into the utils, but don't want utils to be a generic catch all, so I think we could address this separately by making utils.js more specific (maybe renaming to something like messageUtils.js) and combining both into a message specific utility module.
Sure I'll update it with the suggestions |
Thanks! In terms of sorting out the merge conflicts let me know if you need any input there and I'll be glad to provide guidance. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@anontyro just a few minor adjustments and this will be ready to be merged. Thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So close to getting this in, just two more minor adjustments and we'll be there.
src/utils/utils.js
Outdated
@@ -2,11 +2,12 @@ const Discord = require('discord.js') | |||
|
|||
const Utils = { | |||
embedMessage(title, author, color, description) { | |||
console.log(author) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please remove this diagnostic log message.
src/utils/utils.js
Outdated
return new Discord.MessageEmbed() | ||
.setTitle(title) | ||
.setColor(color) | ||
.setDescription(description) | ||
.setFooter(author) | ||
.setFooter(author.username, author.displayAvatarURL) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
displayAvatarURL
is a function that must be called to get the avatar's URL. Please adjust this line to be:
.setFooter(author.username, author.displayAvatarURL())
Hi @anontyro, if you're unavailable to make those last two changes, would you mind checking this option within your pull request screen to give me the ability to add commits to your branch. Then I'll make the changes, commit and push them to your branch and get this merged. Thanks! |
Notable changes:
NaN
but removing the first element changed to safely default to the first item instead