Closed cyyynthia closed 5 years ago
First, thank you so much for reviewing my thing! You've helped a lot <3 This was actually my first time using React.JS! Sorry if it's not professional.
I've fixed most of what you said in my newest commit. But I have a few questions:
I saw you mention this.props.getSetting
/toggleSetting
/updateSetting
. I just updated Powercord with git clone
but am having trouble. I searched through Powercord's source code and saw they're defined with this.settings.connectStore()
. Even after putting this.settings.connectStore(this)
in index.js
, this.props.getSetting
is always undefined from inside Settings.jsx
. Am I missing a step?
About the system I made to show a "refreshing" animation when you add or delete an alias. I did it for a specific reason. I tried absolutely everything I could think of, including forceUpdate
and setState
in Settings.jsx
, and even trying to force-unload-and-reload the plugin. Even after a whole day, I couldn't fix it. Here's what it looks like:
The "New Alias" box keeps its previous values (minor), and after deleting an alias, any aliases following it will not update (major)! I did as much as I could, but right now this refreshing system is the only solution I can see.
Do you remember how I had Discord's class names in a lot of my elements? The point of that was to let people's themes style my settings screen easily, and without making any special exceptions or selectors. In my latest commit, I styled them manually in style.css
, but also added the Powercord selector names to them (such as pc-card
) for the same purpose. Is that okay?
I think that's all I wanted to ask! Thank you again! I hope I'm not being troublesome or anything. Powercord's great btw <3
To get access to the 3 props I mentionned you need to pass the raw component in registerSettings (Settings instead of () => React.createElement(Settings). I'll review again soon and bring back a list of fixes you can make. Thanks for the feedback regarding Powercord ❤️
Review version 2.0 I also added checkboxes so you can check stuff if you want to
key
attribute. That's what causing everything to break. key
must be a unique value, like the alias name for examplereturn
is also useless if you remove thoseNote that you can use random Discord classes, just dynamically grab them using getModule
Hello again! I fixed everything you said!
L15: Just pass Settings to get access to the 3 settings props
L7-9, L25 and L28: you should inline this.pairs fetching in command handling so you don't need to manually update it, it'll be automatically done L71: Raw classes
L31-35: you can remove all of those {}. return is also useless if you remove those
L8: Everything in the constructor will become useless. Can remove it entirely L25: add a key attribute. That's what causing everything to break. key must be a unique value, like the alias name for example
I also added support for Light Mode in Commits 3 and 4, which I forgot to do before :)
It took me a while to figure out how to do the last one, but then I found a helpful page that taught me that key
is a special component in React.js elements. I didn't know, as I'm still pretty new to React.js.
I'm so glad I found it! Thank you so much for helping me!
Hi, I reviewed your plugin and there is a few changes that must be done before being able to get a repo on the community org. I tried to provide as much details as possible, as well as some suggestion to improve code quality. Feel free to reply to this issue or ping me on Discord (Bowser65#0001) if you have any questions/fixed all of those! :D
Also english is not my native language, so there might be some mistakes :p
General
Manifest
repo
key is deprecated and shouldn't be usedpc-commands
dependency is uselsssindex.js
powercord.pluginManager.get('alias')
can and should be replaced bythis
. You should also specify a default value (2nd argument)alias
as primary command name. Conflicts on the main command name will prevent the command from being registered, but alias conflicts will just make the alias ignored. Asa
is very common it have more chance of conflictingSettings.jsx
class
should be replaced byclassName
in JSXthis.props.getSetting(setting, defaultValue)
: grabs a settingthis.props.toggleSetting(setting)
: toggles the setting value (booleans)this.props.updateSetting(setting, newValue)
: update a setting note that any update made to the settings will make your component re-render with new settings automaticallythis.displayItems()
instead of creating a variable<br/>
tag. It works, but you should use CSS margins insteadPair.jsx
getModule
or copy the Discord style in your cssclass
should be replaced byclassName
in JSXPair
)<br/>
. You should use css margins instead<component/>
vs<component></component>
)B
and remove the div children (pass only text)color={Button.Colors.RED} look={Button.Looks.OUTLINED}