UI: an Input ModeViewControl

[nhaagen]

We do have a mode view control in the non-input view controls, but we do not for the input ones.
That's obviously missing.

Styling is missing from this PR, but should not be hard to do for @catenglaender , e.g.

[klees]

Hi @thibsy and @Amstutz,

could you have a look here?

Kind regards!

[thibsy]

Hi @nnhaagen,

Thx a lot for your contribution to the UI framework.

This review will only focus on the public interface, so this can go to JF first. We will review the implementation once it has been accepted.

About the concrete changes, answer the following questions:

• Legacy ViewControl\Mode: the proposed interface looks a lot smaller than the old one. Is this on purpose? I distinctly remember an issue which has been addressed for the old ones, which should probably be considered here too. Something about the label. Also, why not adopt the old factory description?

Please implement the following changes:

• PHPDoc: please provide a PHPDoc annotation for the $options parameter, describing the possible values (corresponding to Mode::isClientSideValueOk()).
• Deprecation: please deprecate the legacy ViewControl\Mode viewcontrol (on the factory).

Kind regards,
@thibsy

[nhaagen]

Hi @thibsy,

• Legacy ViewControl\Mode: the proposed interface looks a lot smaller than the old one. Is this on purpose? I distinctly remember an issue which has been addressed for the old ones, which should probably be considered here too. Something about the label. Also, why not adopt the old factory description?

There's not much beside the options a consumer could or should do with the control itself.
I think that the public interface of the older control is too broad, too (in terms of getAriaLabel/getLabeledActions), but
the main difference is that with the input view controls we have a container properly tending to states and values.

• Deprecation: please deprecate the legacy ViewControl\Mode viewcontrol (on the factory).

Ultimately: yes. But then we should deprecated all of them, aye?
The non-input view controls are still used, e.g. by PresentationTable.

Best regards,
Nils

[thibsy]

Hi @nhaagen,

Thx for your explanation. I agree that we should deprecate the entire legacy view control namespace at once. Could you create a roadmap entry for this? If you have the time, another PR would be highly appreciated of course.

I also realised, I was thinking about the wrong labelling issue (it concerned C\Table\Column\Column::getOrderingLabels()) - sry about that.

The interface LGTM now, so we can pass this through JF. The implementation contains some minor issues, which need to be addressed once this has been accepted.

Please consider the following suggestions. You do not need to follow them:

• Inline JavaScript: we try to keep inline JavaScript to a minimum. I suggest to create a file with a function that contains the logic registered inside the renderer, which is populated as e.g. il.UI.viewcontrol.createMode() or similar.

Please implement the following changes:

• Probable bug: you have several usages of array_keys($options)[0], which I think will break if one passes an empty array to create an instance. I am not sure if we really want to enforce an array entry, therefore I suggest to simply address these places by using array_key_first() with a null-coalesce where necessary.
• Optional ID: the template file contains an optional block for the ID attribute. The attribute is always required though, therefore please remove it (not the variable).

Kind regards,
@Amstutz and @thibsy

[nhaagen]

Hi @thibsy ,

• Inline JavaScript: we try to keep inline JavaScript to a minimum. I suggest to create a file with a function that contains the logic registered inside the renderer, which is populated as e.g. il.UI.viewcontrol.createMode() or similar.

There already is a roadmap entry on this (concerning all other VCs, too), I'll see to it.

• Optional ID: the template file contains an optional block for the ID attribute. The attribute is always required though, therefore please remove it (not the variable).

Actually, with ModeVC, the id is indeed optional (I removed the other ones, though): the JS is on the buttons it generates, not the component itself.
I'll amend when I externalize the JS.

Best,
Nils

[dsstrassner]

This has been postponed to the next JourFixe.

[matthiaskunkel]

Jour Fixe, 06 JAN 2025: We highly appreciate this suggestion and accept the PR for trunk.

[thibsy]

@nhaagen I assign this back to you while you are refining the JS. Feel free to pass back once this is ready!

[nhaagen]

Hi @thibsy , I externalized the JS, maybe you want to take a look?

[thibsy]

Hi @nhaagen,

Thanks a lot for externalising the JavaScript of all the new view controls!

I assumed the actual JavaScript code of the other view controls was simply copied from the renderer, so I only checked the new mode more closely. Let me know if there was a relevant change in some of the other ones.

About the concrete changes, please answer the following questions:

• Styling: is the styling already covered by the legacy view controls? If not, when/how will this be added?

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

• File names: just for future reference, this is currently optional: but according to our JavaScript code-style, file names must reflect their default export, if there is any - with a few exceptions for e.g. generated files or configs.

Please implement the following changes:

• Event.srcElement: this property has been deprecated, please use Event.target instead.
• Optional chaining: please use ?. for all chained function calls when setting the optValue.

Kind regards,

@Amstutz and @thibsy

[nhaagen]

This is what it looks like:

[nhaagen]

Optional chaining: please use ?. for all chained function calls when setting the optValue.

Rollup does not like that chaining:
RollupError: src/mode.js (44:8): The left-hand side of an assignment expression must be a variable or a property access.

In this case, I'd also not expect any of the elements to not be there, they are quite integral to the view control.

[klees]

@thibsy Please iterate =)

[thibsy]

This LGTM now, you're right about the invalid syntax of course. Thx a lot @nhaagen for this contribution!