UI: introduce secondary prompt content

[schmitz-ilias]

As discussed in #11105, this PR is a proposal for introducing secondary content to the show state of prompts. Secondary content is rendered in the prompt below the primary content. The primary content alone determines title and actions of the prompt. Further, ScaleBar is tagged isPromptContent, and the implementation of the "Publish Button" in MetaData is changed accordingly to not use a roundtrip modal as prompt content anymore.

I also included a small fix in the js of prompts, I kept getting an "getElementsByTagName(...).forEach not a function" error. Not sure why this isn't an issue in 11.

In contrast to what was suggested in #11105, I decided to discard actions contributed by secondary content: It should never be necessary to distribute the actions one wants to appear in the prompt over multiple components, and I also wanted to discourage doing silly things like adding multiple forms to the same prompt. If we want to be more strict about this, a bespoke interface isSecondaryPromptContent/isNonInteractivePromptContent could be a way to go.

In the future, I could see demand for being able to also display secondary content above the primary content. Instead of being explicit about this in the signature of Prompt\State\Factory::show, I think it would be best to decide the position of secondary content dynamically based on the component, so e.g. MessageBox always goes on top. In any case, this didn't seem essential for a first implementation, so for now all secondary content goes on the bottom.

I would have added unit tests for this feature, but there don't seem to be any unit tests for prompts at all so far. I also haven't added a KS example. As far as I understood from the discussion in #11105, you don't want to encourage the use of ScaleBar as prompt content, and other combinations of primary above secondary content with the available isPromptContent components also didn't seem suited for an example.

Let me know what you think!

Best, Tim

[schmitz-ilias]

Hi @thibsy, any progress here? The beta for ILIAS 12 is approaching fast.

[thibsy]

@schmitz-ilias I will look into this and the confirmation prompts this week.

[thibsy]

Hi @schmitz-ilias,

Thanks a lot for this contribution – much appreciated!

Information about reviews: read namespaces ILIAS\UI\Component as C and ILIAS\UI\Implementation\Component as I throughout this review/discussion and feel free to do the same.

Your proposal is already very sound, thx for your work and thoughts on this. I support your decision about using only the primary prompt content to derive actions and titles. I also share your stance about determining the placement of (secondary) prompt contents by their semantics, and I would actually like to expand on this already (details below).

Please consider the following suggestions. You do not need to follow them, but please indicate shortly why you prefer to do otherwise:

• I\Prompt\IsPromptContentInternal: if you have the time, I actually think we could get rid of this interface (and implemented methods). This was bugging me for some time now and I think we should derive this information inside the renderer. This must be done for forms already and now that we need to determine locations of secondary content and treat C\MessageBox\MessageBox explicitly as well, we should just handle all of this inside the renderer. If you feel differently, let me know of course!
• C\Chart\ScaleBar: could we only implement C\Prompt\IsPromptContent rather than extend from this on the public interface? I know that IDE's won't be happy about this, but it's going to work. This way we would hide the fact that scale bars are prompt content and discourage further use. If you feel strongly about this let me know.

Please implement the following changes:

• I\Prompt\State\State I: could we accept the initial secondary prompt content inside the constructor already?
• I\Prompt\State\State II: I think to encourage thoughtful compositions we should remove the withContent() mutator and update the withSecondaryContent() so it only appends. Maybe rename to withAdditionalPromptContent() or similar then.
• I\Prompt\State\Renderer: to make #10031 viable we already need to determine secondary prompt content locations. Could you implement a top section that renders any C\MessageBox\MessageBox above the primary content and everything else below?
• C\Prompt\State\State: just for completeness, could you add the withTitle() to the public interface? If we remove I\Prompt\IsPromptContentInternal this will be necessary anyways.
• Unit tests: thx for pointing out that we miss unit tests for this component =). Since there are none, I'm not going to ask you for all of them, but could you implement one that covers the different placements of secondary prompt content and one for the behaviour of I\Prompt\State\\State::withSecondaryContent()?
• Bugfix: could you create and push a separate commit for the bugfix? This way I can easily cherry-pick this change to other supported versions. While you're at it, a third one for amending usages would be perfection =).

Thx for your patience and looking forward to your results!

Kind regards,
@thibsy (as UI coordinator)

[schmitz-ilias]

Hi @thibsy, I implemented the changes, including the removal of I\Prompt\IsPromptContentInternal. The title of the state is now exclusively set via State::withTitle, and buttons via I\Prompt\State\Renderer::deriveButtonsFromPrimaryContent. The latter might get a bit crowded depending on how many components are tagged as prompt content in the future, but most of those will probably only require a call to some getActions method, and no further special handling like for forms.

For unit tests I kept to established patterns in the UI component. Not sure how you want to handle testing of the output of renderers in the future, but asserting the exact HTML seems to be the only way to do it right now. I think there might be a way to at least mock out the rendering of the close button (use a mock I\Button\Factory to return a stub close button, pass that in as $with_stub_rendering when getting the renderer), but that seemed like too much overhead for a very slight improvement in coupling between unit tests.

I'll be unavailable next week, so if this is blocking the implementation of confirmation prompts and you are pressed for time, feel free to make any minor revisions yourself.

Best, Tim

[thibsy]

Hi @schmitz-ilias,

Thx for implementing the requested changes and my suggestion on IsPromptContentInternal.

I agree that without this interface it might become a little bit crowded inside the renderer, but I'm willing to see where this path leads =). Thx for pointing it out.

I disagree that additional mocking is too much overhead for removing the coupling of unit tests, even for simple HTML structures. The overhead you are comparing against is literally every HTML change to the standard button in the future. I wrote about how decoupled rendering tests are implemented in this chapter of the readme.

If you still find the time today, I would appreciate if you:

• StateTest: could improve these rendering tests so that you either decouple the button HTML with stubs, or only assert that a specific part of the HTML is compared, as this would also decouple the test case implicitly since you could simply not compare the button HTMl. For the later we usually use sha1(<component>::class) to reduce risk of false-positives.
• Potential bug: I left an inline suggestion that removes the risk of unexpected nested array inside the new I\Prompt\State\Renderer::deriveButtonsFromPrimaryContent() method.

Otherwise this LGTM, thx!

Kind regards,
@thibsy (as UI coordinator)

[schmitz-ilias]

There you go. Good catch with the nested arrays, I didn't catch it because it works either way. The renderer seems to handle this sort of thing pretty gracefully.

Best, Tim

[thibsy]

Thx a lot! Everything's in order now. I wish you a happy vacation =).