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

document the poll result message and embed #7050

Merged
merged 8 commits into from
Aug 16, 2024
Merged

Conversation

anderswift
Copy link
Member

Documents the poll result message and embed.

@anderswift anderswift force-pushed the document-poll-result branch from 1a0a738 to ced77a3 Compare August 2, 2024 20:07
@anderswift anderswift marked this pull request as ready for review August 2, 2024 20:10
@anderswift anderswift requested a review from a team as a code owner August 2, 2024 20:10
@anderswift anderswift requested review from colinloretz and DV8FromTheWorld and removed request for a team August 2, 2024 20:10
docs/resources/Message.md Outdated Show resolved Hide resolved
docs/resources/Message.md Outdated Show resolved Hide resolved
Co-authored-by: val.le <81811276+valdotle@users.noreply.github.com>
Comment on lines 355 to 362
- `poll_question_text` contains the question text from the original poll message.
- `victor_answer_votes` contains the number of votes for the answer(s) with the most votes.
- `total_votes` contains the total number of votes in the poll.
- `victor_answer_id` contains the id for the winning answer, if applicable.
- `victor_answer_text` contains the text for the winning answer, if applicable.
- `victor_answer_emoji_id` contains the id for the emoji associated with the winning answer, if applicable.
- `victor_answer_emoji_name` contains the name of the emoji associated with the winning answer, if applicable.
- `victor_answer_emoji_animated` specifies whether the emoji associated with the winning answer is animated, if applicable.
Copy link
Contributor

Choose a reason for hiding this comment

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

should we put this in a table so it's more readable?

Copy link
Contributor

Choose a reason for hiding this comment

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

Would be cleaner. Yes please 🙏

Copy link
Contributor

Choose a reason for hiding this comment

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

should we put this in a table so it's more readable?

I gave this whole documentation some more thought, and I think it could use some major changes:
I agree a table would probably be better than this list. But I think the whole embed documentation warrants a separate section and probably a code sample in addition to the table, since this format / way to use embeds (thankfully) isn't widespread.
This, in combination with some improvements to the wording would go a long way to clarify how poll result embeds are structured:
"embed that can have the following fields" should probably be changed to something like "embed field objects that can have the following names/entries/..." in combination with some mention of key-value pairs using the embed field name and value fields respectively.
From my understanding, the whole message reference content attribution section acts as a list of how different message types utilize the message reference object and not as an info dump. Hence why I'd consider the majority of the poll result message documentation off-topic for this section...

Copy link
Contributor

Choose a reason for hiding this comment

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

yeah this part about the embed fields should be in a new section in the embed object documentation. and we should add the automod embeds there later

Copy link
Member Author

Choose a reason for hiding this comment

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

sorry, i've been on vacation but just made some updates based on this feedback. i do agree that the specific embed fields for each type warrants a new section. i didn't want to take on documenting fields for some of the other embed types here, but i can at least get it started and we can flesh this out in another PR.

@anderswift anderswift force-pushed the document-poll-result branch from 65b83a1 to 144fe78 Compare August 15, 2024 02:18
@@ -510,6 +517,23 @@ Additionally, the combined sum of characters in all `title`, `description`, `fie

Embeds are deduplicated by URL. If a message contains multiple embeds with the same URL, only the first is shown.

#### Embed Fields by Embed Type

Certain embed types are used to power special UIs. These embeds use [fields](#DOCS_RESOURCES_MESSAGE/embed-object-embed-field-structure) to include additional data in key-value pairs. Below is a reference of possible embed fields for each of the following embed types.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Certain embed types are used to power special UIs. These embeds use [fields](#DOCS_RESOURCES_MESSAGE/embed-object-embed-field-structure) to include additional data in key-value pairs. Below is a reference of possible embed fields for each of the following embed types.
Certain embed types are used to power special UIs. These embeds use [embed fields](#DOCS_RESOURCES_MESSAGE/embed-object-embed-field-structure) to include additional data in key-value pairs. Consequently, values will always be strings.
Below is a reference of possible embed fields for each of the following embed types.


###### Poll Result Embed Fields

| Embed Field Name | Embed Field Value Description |
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe a "parse as" (or something like that) column could be added to indicate what the value string will look like.
Additionally, a code sample would probably still help to visualize the explanation given above.

@shaydewael shaydewael self-assigned this Aug 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Missing documentation for poll_results message type Missing documentation for embeds with poll_result type
5 participants