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

Only alert once option #2035

Merged
merged 1 commit into from
Aug 21, 2024
Merged

Conversation

jdevuono54
Copy link

I added onlyAlertOnce option and updated readme file

@GitToTheHub
Copy link
Collaborator

Some things:

  • Why did you formatted the properties table, so the Comment column has in all rows the same width? This is not necessary. Please revert it and add only the new property in this table.
  • Please put your property in the right alphabetically order in the table
  • Please put the default of the value also in www/local-notification.js

Thanks.

@GitToTheHub
Copy link
Collaborator

GitToTheHub commented Aug 21, 2024

Thanks and now you can squash everything to one commit :)
And please add "Android only." to the readme for the property, thanks.

@jdevuono54 jdevuono54 force-pushed the only-alert-once-option branch 2 times, most recently from 98b8090 to 058d708 Compare August 21, 2024 09:38
@jdevuono54
Copy link
Author

Everything should be good now. Thanks for the quick responses! :)

@GitToTheHub
Copy link
Collaborator

Thanks, but the whitespaces of the properties table are in again.

@GitToTheHub
Copy link
Collaborator

Everything should be good now. Thanks for the quick responses! :)

No problem, did you saw my last comment?

@jdevuono54
Copy link
Author

Ahh, sorry, it's my IDE that makes the modification during the commit. I'll fix it as soon as possible.

@GitToTheHub
Copy link
Collaborator

Ok thanks, if your IDE does this automatically, you can make a pr for adding the whitespaces. My IDE does not do this automatically. Which IDE do you use?

@jdevuono54
Copy link
Author

PhpStorm

@GitToTheHub
Copy link
Collaborator

ok thanks

@jdevuono54 jdevuono54 force-pushed the only-alert-once-option branch from 058d708 to 696581a Compare August 21, 2024 10:02
@jdevuono54
Copy link
Author

That should be good ! :)

@GitToTheHub
Copy link
Collaborator

Hi thank you, your IDE changed something on the last line in the readme in L576, please revert this

@jdevuono54 jdevuono54 force-pushed the only-alert-once-option branch from 696581a to 0cfb403 Compare August 21, 2024 10:20
@jdevuono54
Copy link
Author

Sorry ><, do you have any idea when a new release with the new option will be available?

@GitToTheHub GitToTheHub merged commit c68af10 into katzer:master Aug 21, 2024
@GitToTheHub
Copy link
Collaborator

You can use the master-branch to use the new option. Thanks for your PR.

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.

2 participants