-
Notifications
You must be signed in to change notification settings - Fork 635
ComponentsV2 support #3123
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
ComponentsV2 support #3123
Conversation
59f40fd to
778ab88
Compare
|
Clippy failure does not seem like my fault? But this should be complete now. |
|
I forgot the new field for webhooks, will add later. |
Seems like this applies prior to ComponentsV2 too, though the restriction here is that they can only use non-interactive components on non application owned webhooks when sending this query param. This can be implemented separately as this works on link buttons from my understanding and the information on https:discord/discord-api-docs#7368. So... this PR should be done completely and now just needs a review (and fixing whatever funky stuff is going on with our CI?) |
eb81725 to
b45cea9
Compare
|
rebased for review reasons, but due to a timecrunch i haven't added V2 support on webhooks yet, will do in a couple hours when i get back. |
Disregard this comment, i didn't have time to properly check but what was needed was added cleanly in the rebase. |
5ed8ae1 to
de3fe0b
Compare
|
Rebased, history will be a little weird because i removed the macro and stuff entirely from the history, doesn't really matter though, makes even sense while going through. Will refer to unofficial docs and get this in as best of a state I can before monday, when the draft PR for docs will drop. |
|
A little note to myself (i'm about to test if i messed this up) i'm not sure if putting an enum over the media items is required/better, nullablity is not confirmed completely yet and we won't know until the docs on monday, i'll go over everything one last time then and hopefully nail it. |
|
I have gone ahead and updated it to match official documentation in discord/discord-api-docs#7487, I am technically doing undocumented stuff right now as for whatever reason they didn't add the extra fields for the unfurled media item, not sure if thats intentional or not yet. |
|
This feature has now been officially released! I (hopefully) have nailed it now and it should be ready for any reviews before merge! I did remove the loading state in the most recent commit, we don't reflect that for embeds (which i think have one too, under the hood) as the docs do not show this. I will leave it under the unstable feature gate until a little bit after merge (even though its on the |
|
Can we remove the unstable gate? It is expected for next to be unstable, the gate is just for undocumented features. |
So... yeah, not under `unstable` yet but things are just starting to work! Need to refactor some old component stuff, and when i put this under the unstable feature I've got some code duplication to handle. Going well so far, sections work great and are really extensive
I do need to look at resolved media stuff, as right now i don't handle that.
This is just a duplicate of MessageFlags and having this duplicate impl for the same flags just adds extra complexity for poise, we don't usually wrap like this in other places like serenity, so I replaced it with MessageFlags
Also changes the modal stuff back to CreateActionRow, no need for this extra layer of nesting if modals only use ActionRow?
|
Merging due to approving review, also tested it myself for about a month and other users have also been using it for a while and have not reported any issues. I'll get a PR ready for poise soon:tm: (or somebody else can do that) |
Opened early, but getting a PR open sooner rather than later doesn't hurt.
TODO:
Component& others more resilient to discord changesunstablefeature flagAdditionally, we really need to PR to current and modify the deserialization to not fail if the top level component is not an ActionRow or it will not deserialize. (See #2963) (Done in #3127)
Once merged, as usual a PR to poise@serenity-next to add support over there too!