Skip to content

Introduce Callout#358

Merged
nilmerg merged 1 commit into
mainfrom
callouts
Jun 3, 2026
Merged

Introduce Callout#358
nilmerg merged 1 commit into
mainfrom
callouts

Conversation

@TheSyscall

@TheSyscall TheSyscall commented Mar 13, 2026

Copy link
Copy Markdown
Contributor

Implement a callout box element that can be used to convey important information to the user.
This element is designed to be used above certain form elements, over the whole form or page.

It seems like the use of callout could conflict with the existing FormDescription and FormNotification decorators, at least within IW2 forms.
Callouts are not meant as a direct replacement for these decorators but as a more universal ipl widget that can be used even outside of forms to convey essential information to the user.

Merging these two concepts by extending the form-notification would be a viable alternative to this PR.

Callout types:
image

Callout full width:
image

Callout with optional title:
image

Callout above a form element:
image

closes #349

requires Icinga/ipl-stdlib#77

@TheSyscall TheSyscall self-assigned this Mar 13, 2026
@TheSyscall TheSyscall added the enhancement New feature or request label Mar 13, 2026
@cla-bot cla-bot Bot added the cla/signed label Mar 13, 2026
@TheSyscall TheSyscall requested a review from flourish86 March 13, 2026 09:59
Comment thread src/Widget/Callout.php Outdated
Comment thread src/Widget/Callout.php Outdated
@flourish86

Copy link
Copy Markdown
Contributor

I like the design, especially the color matching border. The spacing of text and headline are well done.

We should check, though, if there are elements, that have the same or a similar purpose and make sure that the design is consistent.

Please make also sure, that the in this case the line breaks after max. 80 characters to ensure readability.
This is usually done by setting a max width of ~50em to the element containing the paragraph.

@TheSyscall Would be nice, if you could post a screenshot of the element positioned on top of the page.

@TheSyscall

Copy link
Copy Markdown
Contributor Author

On the top of a configuration form without any limit. (limited only by the content)
image

Limited to 80ch/50em.
image

Setting the limit like that breaks the use over form elements.
Note the large blank space on the right side of the callout.
image

flourish86
flourish86 previously approved these changes Mar 24, 2026

@flourish86 flourish86 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

lgtm

Comment thread asset/css/callout.less Outdated
Comment thread src/Common/CalloutType.php Outdated
Comment thread src/Common/CalloutType.php Outdated
Comment thread src/Widget/Callout.php Outdated
Comment thread src/Widget/Callout.php Outdated
@TheSyscall TheSyscall requested a review from Al2Klimov March 24, 2026 12:08
@Al2Klimov Al2Klimov mentioned this pull request Mar 24, 2026
Al2Klimov
Al2Klimov previously approved these changes Mar 31, 2026

@Al2Klimov Al2Klimov left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Tested together with Icinga/icingaweb2#5477 (review).

@Al2Klimov Al2Klimov requested a review from flourish86 March 31, 2026 10:01
@jrauh01

jrauh01 commented May 6, 2026

Copy link
Copy Markdown
Contributor

Would love to see this feature soon, so I don't have to create an own callout widget for Icinga/icingaweb2#5430.

@lippserd lippserd requested a review from jrauh01 May 6, 2026 08:31

@jrauh01 jrauh01 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I tested the different kinds and types and think it looks great. First, please rebase.

Comment thread src/Widget/Callout.php Outdated
Comment thread src/Widget/Callout.php Outdated
Comment thread src/Widget/Callout.php
Comment thread src/Widget/Callout.php Outdated
Comment thread src/Widget/Callout.php Outdated
Comment thread src/Widget/Callout.php Outdated
Comment thread src/Widget/Callout.php Outdated
Comment thread src/Widget/Callout.php Outdated
Comment thread src/Widget/Callout.php Outdated

@jrauh01 jrauh01 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Just some minor phpdocs adjustments.

Comment thread src/Widget/Callout.php Outdated
Comment thread src/Widget/Callout.php Outdated
Comment thread src/Widget/Callout.php Outdated
Comment thread src/Widget/Callout.php Outdated
Comment thread src/Widget/Callout.php Outdated
Comment thread src/Widget/Callout.php Outdated
@TheSyscall TheSyscall requested a review from jrauh01 May 11, 2026 07:55
Comment thread src/Widget/Callout.php Outdated
Comment thread asset/css/callout.less Outdated
Comment thread src/Common/CalloutType.php Outdated
Comment thread src/Widget/Callout.php Outdated
Comment thread src/Widget/Callout.php Outdated
Comment thread src/Widget/Callout.php Outdated
Comment thread src/Widget/Callout.php
@TheSyscall TheSyscall requested a review from jrauh01 May 12, 2026 07:22

@jrauh01 jrauh01 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Consider adding unit tests. Otherwise this looks good now.

Comment thread src/Widget/Callout.php Outdated
@TheSyscall TheSyscall requested a review from jrauh01 May 13, 2026 08:37
jrauh01
jrauh01 previously approved these changes May 13, 2026

@jrauh01 jrauh01 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM

@lippserd lippserd left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

According #358 (comment):

Can you add a note in callout.less that this is a known limitation and max-width is not used in purpose?

setFullWidth(true) and setFormElement(true) can both be applied to the same instance with no documented combined behavior. In the LESS file, both modifiers declare width at equal specificity, so whichever declaration appears last in the stylesheet wins. Is this intentional? Shouldn't this be a mode setter which sets one class exclusively?

Comment thread src/Widget/Callout.php Outdated
'div',
['class' => 'callout-text'],
[
$this->title !== null

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

An empty string title renders a <strong class="callout-title"></strong> with no visible content. Consider adding a trim() !== '' check as in https://github.com/Icinga/ipl-validator/blob/main/src/RegexMatchValidator.php#L36. Also add a test for this. You could also create a PR in ipl-stdlib which adds Str::isEmpty() for future use.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Implemented Str::isEmpty() in Icinga/ipl-stdlib#77.

Tests don't pass because of this.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Reverted

@TheSyscall

Copy link
Copy Markdown
Contributor Author

According #358 (comment):

Can you add a note in callout.less that this is a known limitation and max-width is not used in purpose?

setFullWidth(true) and setFormElement(true) can both be applied to the same instance with no documented combined behavior. In the LESS file, both modifiers declare width at equal specificity, so whichever declaration appears last in the stylesheet wins. Is this intentional? Shouldn't this be a mode setter which sets one class exclusively?

Removed Callout::setFormElement() in favor of using #385 for the use inside a form.

Changed the default behavior to be full-width and renamed the function to change it to setFitContent()

@lippserd lippserd left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Looks good. I will approve once you have cleaned up your commits.

@lippserd lippserd left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Looks good. I will approve once you have cleaned up your commits.

Wait a moment :). Please don’t use Str::isEmpty() in this PR. We can do this later. Else, this PR would require an stdlib release and a composer.json raise.

Comment thread asset/css/callout.less
@TheSyscall TheSyscall force-pushed the callouts branch 2 times, most recently from 929fc7c to 109f841 Compare June 2, 2026 12:31
@TheSyscall

Copy link
Copy Markdown
Contributor Author

Wait a moment :). Please don’t use Str::isEmpty() in this PR. We can do this later. Else, this PR would require an stdlib release and a composer.json raise.

Reverted the use of Str::isEmpty(). I will create a new PR once this is merged.

@TheSyscall TheSyscall requested a review from nilmerg June 2, 2026 13:03
Comment thread asset/css/variables.less Outdated
Comment thread asset/css/callout.less Outdated
@TheSyscall TheSyscall requested a review from nilmerg June 2, 2026 14:25

@nilmerg nilmerg left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM :)

Introduces a Callout widget that renders an information box with a
bordered, tinted background and icon determined by its CalloutType
(Info, Success, Warning, Error). Supports an optional title and a
fit-content sizing mode. Includes LESS variables for theming,
and unit tests.
@nilmerg nilmerg dismissed lippserd’s stale review June 3, 2026 11:21

commits were cleaned up and the requested change applied

@nilmerg nilmerg merged commit 37c58f1 into main Jun 3, 2026
14 checks passed
@nilmerg nilmerg deleted the callouts branch June 3, 2026 11:22
@nilmerg nilmerg added this to the 0.14.0 milestone Jun 3, 2026
nilmerg added a commit to Icinga/icinga-notifications-web that referenced this pull request Jun 3, 2026
For event rules of generic sources a simple `SearchEditor` without
enrichment, validation and suggestions is created.

resolve #450 
require Icinga/ipl-web#384
require Icinga/ipl-web#358
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cla/signed enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Better form hints

6 participants