Skip to content

Conversation

tuj
Copy link
Contributor

@tuj tuj commented Sep 10, 2025

Issue

#249

Description

  • Added screen layout commands.
  • Expanded documentation.

Checklist

  • My code is covered by test cases.
  • My code passes our test (all our tests).
  • My code passes our static analysis suite.
  • My code passes our continuous integration process.

@tuj tuj added this to the 3.0.0 milestone Sep 10, 2025
@tuj tuj self-assigned this Sep 10, 2025
@tuj tuj added the enhancement New feature or request label Sep 10, 2025
@tuj tuj mentioned this pull request Sep 10, 2025
35 tasks
@tuj tuj requested a review from sinejespersen September 10, 2025 07:10
@tuj tuj requested review from turegjorup and removed request for sinejespersen September 10, 2025 07:19
Copy link
Contributor

@turegjorup turegjorup left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This can be merged but I think it would be beneficial to work more cleanly with available vs installed templates and layouts.

Logic for checking if a template or layout is installed is spread througout leading to code that is harder to follow and debug than necessary. I think it should be possible to consolidate that logic in the two service classes. Thus all other code could call ->getInstalledTemplates|Layouts(). This way other code would be more readable .


public function updateScreenLayout(ScreenLayoutData $screenLayoutToUpdate): void
{
if (null !== $screenLayoutToUpdate->screenLayoutEntity) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A comment about what ->screenLayoutEntity signifies and the reason for the null check will help us down the road

@tuj tuj requested a review from turegjorup September 18, 2025 11:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants