[FEATURE] UI: interface for confirmation prompts#12
Open
thibsy wants to merge 2 commits into
Open
Conversation
Author
|
@fhelfer you need to sync trunk to view this PR properly. |
f52dfe1 to
30f541b
Compare
30f541b to
222cf9b
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Hi @fhelfer,
As a result of looking into ILIAS-eLearning#10031 and trying to propose some directions, I already tried to take care of most interface changes.
Before talking about the proposed interface here, let me show you how a "confirmation prompt" could already be implemented right now by making two small changes: 1) make
Prompt\State\Factory::show()accept more than onePrompt\IsPromptContentand 2) make theListing\Entity\Standardsuch a content. Then your endpoint would look like:This endpoint has two major drawbacks though:
So the main goals for my interface were:
The first goal I achieved by passing a combination of
EntityRetrievalandarray<string|int>of entity IDs for creating a new confirmation prompt state. This would actually allows us to omit technical identifiers on theEntityitself, but I think it could be beneficial in the future. It also makes passing along the entity ids internally easier for this state.I know this combination is kind of stupid, because the consumer is in control of both (retrieval and entity ids) and could simply provide an
array<Entity>directly. But there's a thought process behind it: if we shift the request handling closer to the UI framework, it may be possible for the UI framework to take care of fetching the ids from the request. Then we only need an interface for consumers to create entities. In addition, this forces consumers to create an entity retrieval for their component and this will probably lead to more structure.The second goal is a bit hard to measure, but I believe to address it by encapsulating most of the concrete confirmation state behind arguments. I only require the necessary information to build the entity listing, form and message box completely internal. This way we avoid different solutions to the problem and can easily roll out adjustments.
I also simplified the interface of the entity listing, because these two different mappings IMO were complete nonsense. The retrieval now just creates entities directly. This will also simplify the internals, I assume. Since you probably want to use the existing listing and alter it by using context rendering, this should come in handy.
Let me know if you can work with this, or if we should discuss this on Discord.
Best,
@thibsy