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

GH-107 Add configurable notifications #130

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

imDMK
Copy link
Contributor

@imDMK imDMK commented Nov 2, 2023

The configuration file looks now:
image

Tested on localhost.

Additionally, I suggest to remove the SUB_TITLE notification type. And replace it with a standard TITLE notification, which will have the title and subtitle string arguments - if someone wants only the subtitle, they will leave the title message empty and the same with the title. Give your opinion.

Copy link
Member

@CitralFlo CitralFlo left a comment

Choose a reason for hiding this comment

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

I like shortcut from .sendMessage to just .send for the announcer

Copy link
Member

@vLuckyyy vLuckyyy left a comment

Choose a reason for hiding this comment

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

How will the config behave when someone updates eternalcombat to a previously existing config?

@imDMK
Copy link
Contributor Author

imDMK commented Nov 13, 2023

How will the config behave when someone updates eternalcombat to a previously existing config?

It will restore the default value from the configuration.

@Rollczi
Copy link
Member

Rollczi commented Nov 14, 2023

It would be useful to standardize the format of notifications, as here -> https://docs.eternalcode.pl/eternalcore/notifications
I wanted to help you with this, but okaeri configs don't work with junit (for bukkit configurer)
image

First I will refactor dependencies to make it easier to add anything.
(I need mockito or okaeri configurer for yaml)

@Rollczi Rollczi removed their request for review November 24, 2023 18:19
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.

7 participants