Skip to content

Improve ContactForm #301

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

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

Improve ContactForm #301

wants to merge 8 commits into from

Conversation

sukhwinder33445
Copy link
Contributor

@sukhwinder33445 sukhwinder33445 commented Feb 17, 2025

@sukhwinder33445 sukhwinder33445 self-assigned this Feb 17, 2025
@cla-bot cla-bot bot added the cla/signed CLA is signed by all contributors of a PR label Feb 17, 2025
@sukhwinder33445 sukhwinder33445 marked this pull request as draft February 17, 2025 19:21
@sukhwinder33445 sukhwinder33445 marked this pull request as ready for review February 20, 2025 15:07
@sukhwinder33445 sukhwinder33445 force-pushed the improve-contact-form branch 2 times, most recently from 8c775fd to ffe680c Compare February 21, 2025 08:58
@sukhwinder33445 sukhwinder33445 force-pushed the improve-contact-form branch 4 times, most recently from afe1b01 to 65928a3 Compare May 8, 2025 11:48
@sukhwinder33445 sukhwinder33445 requested a review from nilmerg May 9, 2025 09:12
@sukhwinder33445 sukhwinder33445 force-pushed the improve-contact-form branch 3 times, most recently from 15afc4c to c57238f Compare May 15, 2025 11:21
@sukhwinder33445 sukhwinder33445 removed the request for review from ncosta-ic July 23, 2025 11:21
@@ -112,7 +112,7 @@ public function addAction()
$form->getValue('name')
)
);
$this->redirectNow(Links::channels());
$this->redirectNow('__CLOSE__');
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
$this->redirectNow('__CLOSE__');
$this->switchToSingleColumnLayout();

TemplateString::create(
$this->translate(
'No contacts found. To add a new contact, please {{#link}}configure a Channel{{/link}} first.'
.' A default channel is required for the contact.'
Copy link
Member

Choose a reason for hiding this comment

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

What is the reasoning to include?:

A default channel is required for the contact.

The time the contact form first appears, this message is gone and the user forgot what it mentioned because he had to click on Add Contact first to actually see the form.

To me, it just adds complexity to the message.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I see.

Comment on lines 140 to 141
"Link existing Icinga Web users. Users from external authentication backends"
. " won't be suggested and must be entered manually."
Copy link
Member

Choose a reason for hiding this comment

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

We add a description now, but don't explain what the purpose is to link Icinga Web users? Missed opportunity, I'd say.

}

input.search {
padding-left: 1.5em; // property was overwritten .icinga-controls
Copy link
Member

Choose a reason for hiding this comment

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

Without an icon, this padding serves no purpose. Plus, if we really want the icon, this should be standardized somewhat, so a new element type or behavior (completion) should be provided by ipl-web.

Comment on lines 179 to 180
"Contact will be notified via the default channel, when no specific channel is configured"
. " in a schedule or event rule"
Copy link
Member

Choose a reason for hiding this comment

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

Sometimes there's a trailing period, sometimes not. Please clean that up, so all descriptions have one.

]
);

$this->addAddressElements();
$this
->registerElement($defaultChannel)
Copy link
Member

Choose a reason for hiding this comment

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

If you'd register this in the fieldset, you don't have to move the value back and forth later on.

Comment on lines 176 to 179
fieldset[name="contact_address"] {
padding-bottom: 1em;
border-bottom: 1px solid @gray-light;
}
Copy link
Member

Choose a reason for hiding this comment

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

Please use an hr element instead.

.icinga-form p.description {
color: @text-color-light;

&:last-child { // fieldset > .control-group:last-of-type rule unset the last element's bottom margin
Copy link
Member

Choose a reason for hiding this comment

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

If it only applies to fieldsets, then please make sure this rule does so as well!

}
}

.icinga-form p.description {
Copy link
Member

Choose a reason for hiding this comment

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

form p.description should suffice.


if ($backend instanceof DomainAwareInterface) {
$names = array_map(function ($name) use ($backend) {
return $name . '@' . $backend->getDomain();
Copy link
Member

Choose a reason for hiding this comment

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

Image

As the element `default_channel_id` is no longer part of the field set `contact`, it must be handled accordingly
@sukhwinder33445 sukhwinder33445 requested a review from nilmerg July 29, 2025 11:46
@sukhwinder33445 sukhwinder33445 force-pushed the improve-contact-form branch 2 times, most recently from b765153 to 251fde3 Compare July 29, 2025 12:17
@sukhwinder33445 sukhwinder33445 removed the request for review from nilmerg July 29, 2025 12:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla/signed CLA is signed by all contributors of a PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Improve contact configuration ContactForm: Selected default channel element must be required
2 participants