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

[FEATURE] Standardizing Embed Formats #146

Closed
RazCrimson opened this issue Oct 23, 2020 · 7 comments · Fixed by #148
Closed

[FEATURE] Standardizing Embed Formats #146

RazCrimson opened this issue Oct 23, 2020 · 7 comments · Fixed by #148
Assignees

Comments

@RazCrimson
Copy link
Contributor

Is your feature request related to a problem? Please describe.
The Embed function in utils requires a author object to be passed as a parameter. But many locations in the code base pass author.tag into that. Some other locations send the message.author.id . Would like to standardize that to message.author.

Describe the solution you'd like
I would like a Discord.User/message.author instance of the message/command author to be passed as a parameter to the embed generator and access the tag attribute from that and also display the User's Avatar.

Additional context
Current :
image

Would like it to be:
image

@rgroves
Copy link
Collaborator

rgroves commented Oct 23, 2020

This looks like it would be a nice addition and make the message embeds look nicer. What do you think @tomassirio?

I believe Issue #128 which is in the same spirit as this and is already in the works. Let's check with the contributor there to see where there at, because if their close to finishing it would probably be better to apply work for this after their changes. Otherwise, y'all might step over each others work and have to deal with some messy merge conflicts.

@tomassirio
Copy link
Owner

They look really better and @RazCrimson is right about the redundancy in objects passed as parameters

@rgroves
Copy link
Collaborator

rgroves commented Oct 23, 2020

@RazCrimson It looks like Issue #128 isn't able to be worked on currently by the contributor who first requested it. Would you like to work on not only making the footer look better, but making the entire embed look a little better as described in #128? These are very much the same issue so you could tackle them both in a single PR and just mention in your PR comment that it closes them both.

You can do your original suggestion only or both. It's up to you.

This would be a good time to get rid of utils.generateListEmbed It doesn't seem to be needed (it could just as easily use utils.embedMessage and falls in line with standardizing the embed messages.

Thanks!

@RazCrimson
Copy link
Contributor Author

@rgroves Ok, will try doing it.

@RazCrimson
Copy link
Contributor Author

@rgroves Should I add code blocks to embeds? Or should I directly just use code blocks?

Using code blocks within a embed adds a limitation on the length of the list item. It goes beyond the length it wont look good.

@rgroves
Copy link
Collaborator

rgroves commented Oct 24, 2020

@RazCrimson Good job! I've reviewed the changes and for the most part they look good just a couple of adjustments are needed when you have a chance.

My initial thoughts are as follow:

  • Do you know what the length limitation is when using code blocks like that?
  • I see you've chosen Nim as the formatter being used for the code block. If we go this route, maybe a future Issue and PR can address making this configurable (so that the chosen formatter can be pulled form the config object and set in the .env file.

The Nim formatting doesn't look great, but I imagine none of the code formatters will work perfectly for ListBot's purposes.
image

@RazCrimson
Copy link
Contributor Author

@rgroves The description of a embed (where I insert code blocks text) has a length limit of 2048 characters. We might need to split it into multiple messages if it gets too long.

As for making it into a format which can be specified in .env, its totally possible. Will make a new Issue for that after I am done with this, I guess.

We also need:

  • A PR to handle the messages which exceed the max length must be made.

  • Format/pad the list with spaces to make it look better.

I also made some changes to standardize the usages of embeds. Take a look. There seems to be some redundant initialization of variables in the code base which need to be removed too.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants