Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

UI/Legacy: factory->legacy() will return Legacy\Factory, not Component #8436

Open
wants to merge 3 commits into
base: trunk
Choose a base branch
from

Conversation

nhaagen
Copy link
Contributor

@nhaagen nhaagen commented Nov 14, 2024

ILIAS\UI\Factory::legacy used to build the Legacy component directly.
This proposes a Factory for legacy components to open up Legacy for further components.

When working on the SequenceNavigation, we had the need to define a depency for the actual component (the navigation) without previously defining all the inner parts of the dependency (in this case, the Segment).
Right now, there are still a lot of proper components missing, especially for the "inner parts", the content.
Often a simple legacy content is OK, too, but I consider it quite helpful to mark interim compositions with interfaces as not to use "Legacy" as type in the new code. I bet, I'm not all alone with this.

Due to the UI-crawlers I also had to rename Legacy\Legacy to Legacy\LegacyContent.
I tried my best to change all the relevant occurences along, that's why this PR is so huge.

@nhaagen nhaagen force-pushed the 11/UI/Legacy branch 2 times, most recently from 11ce835 to 0ac698c Compare November 14, 2024 13:24
@klees klees self-assigned this Nov 18, 2024
@klees
Copy link
Member

klees commented Nov 18, 2024

Hi @thibsy and @Amstutz,

I asked @nhaagen to change the name from legacy()->legacyContent() to legacy()->content() to remove the duplicate word.

The rest of this is rather mechanic and the general reasoning is not dependent on exact wording, so I kindly ask you to look into this now, so we can keep on iterating on #8393.

Thanks!

@Amstutz
Copy link
Contributor

Amstutz commented Dec 4, 2024

Hi @nhaagen

Thx a lot for this proposition for anybody reading along here and trying to find the relevant changes look here: https://github.com/ILIAS-eLearning/ILIAS/pull/8436/files#diff-fffae9cf25b57b2a183fcfe906fdc503176d04a58f3e2c4935ef79f3b8fd90b9 and here: https://github.com/ILIAS-eLearning/ILIAS/pull/8436/files#diff-d69701639aa685ed4ea193add1371e41bbccf59ff74bcd87b68546cba58287f1 further note that there are changes here: https://github.com/ILIAS-eLearning/ILIAS/pull/8393/files#diff-12d7f598624c32dd8cf6821c891f5dc9d1d9ec2b6fd7f302d634e43bb54fdeaa that will make use of the change being proposed here by introducing a Legacy Segment next to the Legacy Content.

We understand the reasoning and approve the proposed change. Please, everybody, note that this will have breaking changes in plugins using the legacy component. They should be easy to fix, but could be numerous. The changes in core should be fixed with this PR here.

We recommend the JF to approve the proposed change for trunk only.

Kindly
@Amstutz and @thibsy

@dsstrassner
Copy link
Contributor

This has been postponed to the next JourFixe.

@matthiaskunkel
Copy link
Member

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

@nhaagen nhaagen force-pushed the 11/UI/Legacy branch 2 times, most recently from 9afe413 to 4837ee0 Compare January 6, 2025 15:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants