Skip to content

Conversation

distantnative
Copy link
Member

@distantnative distantnative commented Aug 13, 2025

Description

#7522 for seeing the larger context how they should be used.

Merge first

Changelog

🎉 Features

  • New Kirby\Panel\Controller\Dialog\ModelsPickerDialogController classes (Kirby\Panel\Controller\Dialog\PagesPickerDialogController, Kirby\Panel\Controller\Dialog\FilesPickerDialogController, Kirby\Panel\Controller\Dialog\UsersPickerDialogController)

For review team

  • Add changes & docs to release notes draft in Notion

@distantnative distantnative self-assigned this Aug 13, 2025
@distantnative distantnative force-pushed the v6/refact/models-picker-dialogs-controllers-classes branch 3 times, most recently from 68141f7 to 08ae026 Compare August 13, 2025 08:38
@distantnative distantnative force-pushed the v6/refact/models-picker-dialogs-controllers-classes branch from 08ae026 to 3f0b56d Compare August 13, 2025 08:50
@distantnative distantnative marked this pull request as ready for review August 13, 2025 08:54
@distantnative distantnative requested a review from a team August 13, 2025 08:54
Copy link
Member

@bastianallgeier bastianallgeier left a comment

Choose a reason for hiding this comment

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

I really like the refactoring. The props in the constructor make me wonder though if this is something that we should do for other controllers as well, where we often go directly through request params instead.

@bastianallgeier bastianallgeier added this to the 6.0.0-alpha.1 milestone Aug 13, 2025
@distantnative
Copy link
Member Author

@bastianallgeier I can't quite follow your thought - what is it what you are suggesting? 😅

@bastianallgeier
Copy link
Member

@distantnative 😆 sorry for the confusion. Let's take the PageChangeTitleDialogController. It maybe not be the best example, but the first one I found where we use the request in the load method to get the 'preselect' prop. We normally mostly just pass the Model to the constructor and search for the model in the factory method of the controller. Additional options are then coming from the request object. Would it be better to set those options properties in the constructor as well and then hand them over from the request in the factory method instead?

@bastianallgeier
Copy link
Member

But this discussion is basically not something that we have to decide for this PR. It's more a general conceptual decision that we can still leave open or change later.

@bastianallgeier bastianallgeier merged commit 8283999 into v6/develop Aug 13, 2025
9 checks passed
@bastianallgeier bastianallgeier deleted the v6/refact/models-picker-dialogs-controllers-classes branch August 13, 2025 10:55
@distantnative
Copy link
Member Author

Now understood you. Yes, maybe in the long term they should already be passed to the constructor - I think we need to test out what works best for the most cases once we have done a bit more refactoring (e.g. #7466).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants