-
Notifications
You must be signed in to change notification settings - Fork 14
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
[QuickInstall] Allow Forum Post installation for both Revenge and Vendetta's Plugins / Themes forums #33
Conversation
@@ -464,6 +464,9 @@ interface VendettaObject { | |||
DISCORD_SERVER_ID: string; | |||
PLUGINS_CHANNEL_ID: string; | |||
THEMES_CHANNEL_ID: string; | |||
DISCORD_REVENGE_SERVER_ID: string; | |||
PLUGINS_REVENGE_CHANNEL_ID: string; | |||
THEMES_REVENGE_CHANNEL_ID: string; |
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.
Don't think we need to put type definitions for the new constants here yet, maybe after #29 is completed we can put our own APIs definitions.
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.
Got it, then in that case I'll wait for 29 to be completed and then discuss about it. To be completely honest, I once managed to get it work without defining the constants in def.d.ts, but I don't know what black magic there was.
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.
Oh no, #29 isn't blocking this PR. The type definitions provided in the d.ts file is just for generating the types package IIRC. It should work without the new type additions.
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.
Runtime side looks good
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.
maybe turn THEMES_REVENGE_CHANNEL_ID
and PLUGINS_REVENGE_CHANNEL_ID
into arrays that includes both Vendetta and Revenge channel ids?
@@ -25,17 +28,24 @@ const { hideActionSheet } = findByProps("openLazy", "hideActionSheet"); | |||
|
|||
export default () => | |||
after("default", ForumPostLongPressActionSheet, ([{ thread }], res) => { | |||
if (thread.guild_id !== DISCORD_SERVER_ID) return; | |||
if (thread.guild_id !== DISCORD_SERVER_ID && thread.guild_id !== DISCORD_REVENGE_SERVER_ID); |
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.
You removed the return
, meaning this if statement does nothing now.
@@ -3,9 +3,12 @@ export const DISCORD_SERVER = "https://discord.com/invite/ddcQf3s2Uq"; | |||
export const DISCORD_SERVER_ID = "1015931589865246730"; | |||
export const PLUGINS_CHANNEL_ID = "1091880384561684561"; | |||
export const THEMES_CHANNEL_ID = "1091880434939482202"; | |||
export const DISCORD_REVENGE_SERVER_ID = "1205207689832038522"; |
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.
Why not change this to an array while we are at it?
// Adds Vendetta and Revenge as valid servers
export const DISCORD_SERVER_IDS = ["1015931589865246730", "1205207689832038522"];
export const PLUGINS_CHANNEL_IDS = ["1091880384561684561", "1205876359037980742"];
export const THEMES_CHANNEL_IDS = ["1091880434939482202", "1205876424188104715"];
@@ -25,17 +28,24 @@ const { hideActionSheet } = findByProps("openLazy", "hideActionSheet"); | |||
|
|||
export default () => | |||
after("default", ForumPostLongPressActionSheet, ([{ thread }], res) => { | |||
if (thread.guild_id !== DISCORD_SERVER_ID) return; | |||
if (thread.guild_id !== DISCORD_SERVER_ID && thread.guild_id !== DISCORD_REVENGE_SERVER_ID); | |||
|
|||
// Determine what type of addon this is. | |||
let postType: "Plugin" | "Theme"; | |||
if (thread.parent_id === PLUGINS_CHANNEL_ID) { |
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.
When you change the variables to be arrays these if-statements would be:
if (PLUGINS_CHANNEL_IDS.includes(thread.parent_id)) {
...
}
Ah didn't see your review :) @puhbu I've made a PR on your branch with those changes ^^ |
Firstly, yes, I know my commits are cancerous
and so is my codeAllow the user to install themes and plugins by holding down on forum posts on both Vendetta's server and Revenge's server.
Worth noting:
Things To-do:
I'm open to any comments. I also dislike my code but there's always room for improvement.