ILIAS icon indicating copy to clipboard operation
ILIAS copied to clipboard

11/UI: Sequence Navigation

Open nhaagen opened this issue 1 year ago • 8 comments

Screenshot from 2024-11-11 13-58-35

This adds to the UI:

  • a "Navigation" section
  • a sequence player

The Sequence Navigation uses the same builder-pattern known from ui-tables: consumers will have to provide a binding defining possible positions of the sequence (that has to be done at one point, anyways) and build a view/page/... = Segment with allowed contents.

Operating the sequnce will then display the segments one by one, also allowing for view controls as well as global and segment-specific actions.

This PR includes https://github.com/ILIAS-eLearning/ILIAS/pull/8436

nhaagen avatar Nov 11 '24 13:11 nhaagen

Hi @nhaagen,

please rebase.

Kind regards!

klees avatar Jan 08 '25 15:01 klees

Hi @Amstutz and @thibsy,

could you please move this forward? This is good to go from our perspective...

Kind regards!

klees avatar Feb 24 '25 13:02 klees

Hi @nhaagen

Thank you a lot for contributing to ILIAS.

I am very happy that this component has found its way into the UI framework. Many thanks for your efforts and those of others involved here.

Note: The following statements focus on visual and interaction design, as well as the descriptive texts of the “Expected Output” and a short HTML Validation test (the same as we use in core testing). The statements refer to the version of the CaT test platform provided to me (ILIAS v11.0_alpha). Many thanks for providing this platform.

Please answer the following questions:

  • [x] Why are the buttons of the primary navigation and ViewControls displayed one below the other? I seem to remember that @catenglaender had worked out a suggestion here with a styled sticky primary navigation. Should that be part of this or was it or the styling intentionally omitted? Bildschirmfoto 2025-04-07 um 10 02 12

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

  • [x] The HTML Validator returns a few errors, but not all of them affect the component. I tried to extract the ones that belongs to the Sequence Navigation. Please check which of these apply to the Sequence Navigation component and can be corrected.
Error: Duplicate ID .

[From line 5590, column 21; to line 5590, column 69](https://validator.w3.org/nu/#l5590c69)

ss="well"><div class="c-sequence c-sequence--linear" id="">
Error: Bad value for attribute id on element [div](https://html.spec.whatwg.org/multipage/#the-div-element): An ID must not be the empty string.

[From line 5590, column 21; to line 5590, column 69](https://validator.w3.org/nu/#l5590c69)

ss="well"><div class="c-sequence c-sequence--linear" id="">

Please implement the following changes:

  • [x] The text “expected output” refers to the representation of a "Characteristic Value Listing". In the meantime, a highlighted placeholder is displayed. Please adapt the text accordingly, e.g. “ILIAS shows a group of buttons and a placeholder for the segment”.
  • [x] The button “a segment action for pos x” is missing in the text of expected outputs. Please add this button to the description. It could be helpful to add the sentence that the label adjusts at position 2 to support the fact that this button belongs to the corresponding segment.
  • [x] Small linguistic adjustment in the text of expected output: Please change “vieww control” to “view control”.

As soon as the above question about the display of the primary navigation has been clarified, I will be happy to provide further feedback. Thanks!

kindly, @yvseiler

yvseiler avatar Apr 07 '25 08:04 yvseiler

This is how the component looks now - if it looks any different on dev servers and environments, delos.scss needs to be recompiled and placed in the public/assets/css folder (my latest commit includes an up to date compile in templates/default/)

image

  • Following up on some conversations I had both in-house and at the DevConf, the navigation has been moved back up to the header. This is to strengthen the Sequence Navigators main purpose of stringing together content without triggering an action. (Future child components and players might have a different focus and therefor make different decisions about the visual hierarchy.)
  • several accessibility markers have been set to improve the UX with screen readers - this includes description texts which required new translation strings.
  • screen readers have a general problem with our sr-only class and mixin. When space is too limited for the invisible div to spread, Orca on Ubuntu stops reading after a forced linebreak. Setting 'white-space: nowrap' on sr-only remedies this.
  • tasks from @yvseiler's feedback are done

So with regards to rendering, I hope we are near the finish line now. :slightly_smiling_face:

catenglaender avatar Apr 08 '25 11:04 catenglaender

Hi @thibsy and coordinators, thanks for digging through this.

  • [ ] C\Navigation\Sequence namespace: could you explain why this level of hierarchy is necessary or beneficial?

I think it is very likely that other navigation-elements will follow; as examples, I added Breadcrumbs and Menu to the roadmap, but I expect even more to come.

  • [ ] Binding::getSequencePositions(): could you explain why this method is necessary? Why not introduce a ::getSegments() method that returns a Generator<Segment>?

Segments might be quite huge, with a lot of dependencies and queries to the db; for the navigation itself, this is irrelevant but for the actual to-be-rendered segment. I admit that the example suggests otherwise, however, only an array of identifiers is actually needed.

  • [ ] Request coupling: we need to accept an URLBuilder and URLBuilderToken in the constructor of a C\Navigation\Sequence\Sequence component, for generating the URLs. You currently require consumers to always provide a request because you need to derive actions from the current request URL. I think this is something we should avoid in general, because consumers should be able to control URLs without having to rely on the current request URL. I understand we still need the withRequest() call for applying view controls, but we should do as little with the request as possible here.

We had some internal discussion about this. I understand the issue, but would still refrain: The navigation itself should stay "in place", i.e. just operate on the view it was rendered in. I don't see any reason, though, to determine that place during construction, and so pin the navigation to a single view. As you say, at least when view controls are involved, the request is needed anyways, and it is sort of unnecessarily cumbersome to manually create the URLBuilder. The Segments, however, may still be delivered entirely unrestricted.

  • [ ] C\Navigation\Sequence\Binding: please rename this interface to SegmentRetrieval and maybe move it up one hierarchy level, depending on question above.

I'll do that :)

  • [ ] C\Legacy\Segment::withSegmentActions(): it looks like this method rather belongs to C\Navigation\Sequence\Segment. Please move the method there and remove the redundant "segment" wording.

Actually, Navigation\Sequence\Segment only needs to read actions; I'd reckon, they are usually built internally upon already existing ones. The Legacy\Segment, however, might well add some, since there is no actual exisiting content. In the end, exisiting components should implement Navigation\Sequence\Segment.

Best regards, Nils

nhaagen avatar Apr 09 '25 07:04 nhaagen

Jour Fixe, 28 APR 2025: We highly appreciate this suggestion and accept this new UI element for trunk.

matthiaskunkel avatar Apr 28 '25 12:04 matthiaskunkel

Not even merged yet, but already a change on the interface: When I was using the Sequence with a Form in a Segment, there were certainly several ways to get the request to the Retrieval somehow, but it felt quite weird to do so. Also, this seems to be a basic shortcoming that will occur every time we build segments with some sort of interaction. E.g, considering a form failing its validation, the form should be displayed again, highlighting the errors. The binding has to take care of that when building the Segment. Considering the wide range of other possible usecases, it's probably best to just hand over the request.

nhaagen avatar May 20 '25 08:05 nhaagen

Hi @thibsy and @Amstutz,

we are currently in the process to actually use this UI component for something (the manual correction in the test). When working on this, it became appearant to @nhaagen, that he would require the Request in a segment, since he wanted to build a form in that sequence.

Since we currently haven't codified any non-legacy-segments, we first wondered if this was an accident due to the use of the legacy segment. This is not the case: Even if, say, the form would be a proper segment, it would require the request to perform processing and, e.g., show errors.

We then discussed options, how the request could be made available in the SegmentBuilder:

  1. Have it as a property on the SegmentBuilder and access it via $this.
  2. Pass it via a construct such as $additional_params as we do with the DataRetrieval for tables.
  3. Pass it as a distinct parameter.

We discarded 1. because the Sequence Navigation itself has a withRequest-method. If the DataRetrieval has a property for the request, these two could get out of sync. Something similar goes for 2., since parameters would need to be bound manually in that case, again with the potential problem of out of sync. With 3., the Seguence Navigation could pass the received Request on its own, which would guarantee they are in sync. Also: Other then e.g., for the Data Table, we expect that the Segments contain some interactivity that is only loosely structured, which would require the Request anyway. Passing the Request then is just the way to make that interactivity generally possible.

Hope this makes some sense.

Kind regards!

klees avatar May 20 '25 12:05 klees

I moved the position handling to the renderer, but cannot say that it looks better than before; especially the missing refinery on the renderer is bugging me.

nhaagen avatar Jul 11 '25 10:07 nhaagen

@nhaagen are you waiting for feedback here? Just making sure we are not in a deadlock. If this is ready for the next round, you can reassign me.

thibsy avatar Jul 22 '25 08:07 thibsy

Hi @thibsy,

regarding I\Sequence\Sequence::withRequest(): I think I see where you coming from, but I would still prefer if we could leave things as they have been before. I had a look into the actual change that was spawned by the change request with @nhaagen.

Maybe you had something different in mind than that, but I think we can observe that the Renderer gains some weight regarding code and knowledge about data processing of the component. To me this is too much. Why, e.g., should the Renderer know how to reach in the query? Code like

        $position = $query->retrieve(
            $token_position->getName(),
            $refinery->byTrying([
                $refinery->kindlyTo()->int(),
                $refinery->always(0)
            ])
        );

looks super accidental regarding the initial task of the Renderer: Turn a component into according HTML. I'd rather have that code in the component itself.

Two other things speak to the previous solution: It is super similar to the way we do things in the forms. Data retrieval and processing is also handled inside the component there. And: Having that code in the component makes it easier to write test for that code.

So, can we put things back the way that they have been before? Even more, code like

        if ($position >= count($positions) || $position < 0) {
            $position = 0;
        }

would also belong into the component from my POV.

Ping me,if we shall discuss this personally.

Kind regards!

klees avatar Jul 30 '25 11:07 klees

Hi @klees,

Thx for your feedback here. I agree, this is probably too much for a renderer.

While thinking about the side-effects again, I also noticed that they may not be implemented in the component anymore, but when rendering the component they still occur. So, I guess getting rid of the side-effects only shifted them to a less obvious location now. Good ol' "Verschlimmbesserung" =).

@nhaagen sorry for the extra roundtrip, you can revert the according change.

Kind regards, @thibsy

thibsy avatar Jul 30 '25 15:07 thibsy

Hi @thibsy , I think this is ready for the next round :) Thanks in advance, Nils

nhaagen avatar Aug 12 '25 07:08 nhaagen

Jour Fixe, 01 SEP 2025: The PR has already been merged. But @thibsy wanted to notify us about the latest interface changes in this PR, see statement by Richard from 20MAY.

matthiaskunkel avatar Sep 01 '25 12:09 matthiaskunkel