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

Merge settings.Notification and settings.CommandFeedback #1711

Open
ccuser44 opened this issue Nov 13, 2024 · 7 comments
Open

Merge settings.Notification and settings.CommandFeedback #1711

ccuser44 opened this issue Nov 13, 2024 · 7 comments
Labels
✨ enhancement Enhancing or improving existing functionality 🖇️ loader The Adonis Loader and loader settings

Comments

@ccuser44
Copy link
Contributor

What part of Adonis is this related to?

Loader/Settings

What are you suggesting?

Merge settings.Notification and settings.CommandFeedback. Both settings are related in functionality and probably should be merged to a single setting for clarity. It really doesn't make sense to have 100% fine grained tuning in exchanged for reduced clarity, especially when it's already a problem for the config.

@ccuser44 ccuser44 added the ✨ enhancement Enhancing or improving existing functionality label Nov 13, 2024
@github-actions github-actions bot added the 🖇️ loader The Adonis Loader and loader settings label Nov 13, 2024
@EasternBloxxer
Copy link
Contributor

This is not true. One command shows feedback for each command you run and other is notifications you get on Join. First one js something people don't want on by default other is something most people want to keep on. This would be a horrible Change.

@ccuser44
Copy link
Contributor Author

This is not true. One command shows feedback for each command you run and other is notifications you get on Join. First one js something people don't want on by default other is something most people want to keep on. This would be a horrible Change.

Right. They are both on by default though. So I'm not really sure if it would be that negative of a chance.
Though I definitely am not going to do this right now, I'm just leaving this issue here to get opinions on the change or if someone decides to implement it.

@EasternBloxxer
Copy link
Contributor

Since when is commandfeedback on by default???

@PurpleCreativity
Copy link
Contributor

This is not true. One command shows feedback for each command you run and other is notifications you get on Join. First one js something people don't want on by default other is something most people want to keep on. This would be a horrible Change.

Right. They are both on by default though. So I'm not really sure if it would be that negative of a chance. Though I definitely am not going to do this right now, I'm just leaving this issue here to get opinions on the change or if someone decides to implement it.

https://github.com/Epix-Incorporated/Adonis/blob/master/Loader/Config/Settings.luau#L280
Fact check your sources

@ccuser44
Copy link
Contributor Author

Oh damn I mixed it up with settings.PlayerCommands when looking if it was on lol

@PurpleCreativity
Copy link
Contributor

settings.Notification is for displaying a notification on-join about your admin level.
settings.CommandFeedback is for notifying users when commands with non-obvious effects are run on them.

Even though they might be a bit related, they should be kept separated.

@ccuser44
Copy link
Contributor Author

ccuser44 commented Nov 13, 2024

settings.Notification is for displaying a notification on-join about your admin level. settings.CommandFeedback is for notifying users when commands with non-obvious effects are run on them.

Even though they might be a bit related, they should be kept separated.

Yeah I think so. I originally thought it just applied to the fly notification and I thought it was on by default. But now thinking further about it I see that it would be a bad idea.

The original pull requests proposed that it may be a wise idea to make them use hints though #630
So maybe we should remove settings.CommandFeedback altogether and convert all of them to use hints? Not sure if that's a good idea but it's a possibility

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
✨ enhancement Enhancing or improving existing functionality 🖇️ loader The Adonis Loader and loader settings
Projects
None yet
Development

No branches or pull requests

3 participants