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

Event rule config enhancement #159

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

raviks789
Copy link
Contributor

@raviks789 raviks789 commented Feb 21, 2024

Use a single form for event rule configuration.

Blocked by Icinga/icingaweb2#5190

@cla-bot cla-bot bot added the cla/signed CLA is signed by all contributors of a PR label Feb 21, 2024
@raviks789 raviks789 force-pushed the event-rule-config-enhancement branch 4 times, most recently from c40476a to 8e16e7e Compare February 28, 2024 09:14
@raviks789 raviks789 force-pushed the event-rule-config-enhancement branch 13 times, most recently from 1c9054f to 7ab548f Compare March 6, 2024 12:35
@raviks789 raviks789 force-pushed the event-rule-config-enhancement branch 9 times, most recently from 25b4a12 to 163112c Compare March 13, 2024 09:32
@raviks789 raviks789 force-pushed the event-rule-config-enhancement branch 3 times, most recently from 97018d5 to 4b1380c Compare March 18, 2024 13:08
@raviks789 raviks789 force-pushed the event-rule-config-enhancement branch 3 times, most recently from 2b31a5a to 4bbe6f0 Compare May 13, 2024 15:24
@sukhwinder33445 sukhwinder33445 self-requested a review May 14, 2024 08:42
@raviks789 raviks789 force-pushed the event-rule-config-enhancement branch from 220a79d to c81fd09 Compare May 14, 2024 12:18
Copy link
Contributor

@sukhwinder33445 sukhwinder33445 left a comment

Choose a reason for hiding this comment

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

The delete button doesn't show any hint why it is deactivated. I can't find the previous comment, but I've requested this change before also.

$buttonsWrapper->add(
[$eventRuleConfigSubmitButton, $discardChangesButton, $deleteButton]
);
$buttonsWrapper->add([$eventRuleConfigSubmitButton, $discardChangesButton, $deleteButton]);
Copy link
Contributor

Choose a reason for hiding this comment

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

This button can then be added conditionally.

application/controllers/EventRuleController.php Outdated Show resolved Hide resolved
application/controllers/EventRuleController.php Outdated Show resolved Hide resolved
application/controllers/EventRuleController.php Outdated Show resolved Hide resolved
application/controllers/EventRuleController.php Outdated Show resolved Hide resolved
application/forms/EventRuleConfigForm.php Outdated Show resolved Hide resolved
application/forms/EventRuleConfigForm.php Show resolved Hide resolved
application/forms/EventRuleConfigForm.php Show resolved Hide resolved
@raviks789
Copy link
Contributor Author

raviks789 commented May 14, 2024

The delete button doesn't show any hint why it is deactivated. I can't find the previous comment, but I've requested this change before also.

This comment is present in PR #170. You are right, it must be fixed here.

application/forms/EventRuleConfigForm.php Outdated Show resolved Hide resolved
application/forms/EventRuleConfigForm.php Outdated Show resolved Hide resolved
application/forms/EventRuleConfigForm.php Outdated Show resolved Hide resolved
application/forms/EventRuleConfigForm.php Outdated Show resolved Hide resolved
application/forms/EventRuleConfigForm.php Outdated Show resolved Hide resolved
@raviks789 raviks789 force-pushed the event-rule-config-enhancement branch 2 times, most recently from 6a76ace to 3dae615 Compare May 15, 2024 12:19
@raviks789 raviks789 force-pushed the event-rule-config-enhancement branch 3 times, most recently from 8b5bffe to be66793 Compare May 15, 2024 14:28
Copy link
Contributor

@sukhwinder33445 sukhwinder33445 left a comment

Choose a reason for hiding this comment

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

Looks good to me, works fine.

@nilmerg please have a look.

@sukhwinder33445
Copy link
Contributor

Please squash the commits.

@sukhwinder33445
Copy link
Contributor

Please do not forget to keep an eye on the phpstan errors.

@raviks789 raviks789 force-pushed the event-rule-config-enhancement branch from d31e8a6 to fd35468 Compare May 21, 2024 15:52
@nilmerg nilmerg removed this from the Beta milestone May 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/view Affects an entire view cla/signed CLA is signed by all contributors of a PR enhancement New feature or improvement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Rewrite Event Rule Configuration
3 participants