-
Notifications
You must be signed in to change notification settings - Fork 25
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
Add single_snippet_selection content type resolver #56
Add single_snippet_selection content type resolver #56
Conversation
Think later it would make sence to create a seperate PR to exclude functions like "getDefaultSnippetId" in own Service to reduce multiple code lines |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks good otherwise. thanks a lot! 🙂
|
||
$resolvedSnippet = $this->structureResolver->resolve($snippet, $locale, $loadExcerpt); | ||
|
||
return new ContentView($resolvedSnippet, ['id' => $data]); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
would you mind to adjust the existing SnippetSelectionResolver
to use this format too? 🙂 (['ids' => ...]
). then this would be consistent for the snippet selections
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like we forget that when fixing #46 same should be done for the PageSelectionResolver and SinglePageSelectionResolver. Then I think all should be consistent.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i guess it would also make sense to use that format in case there is no value set 🙂 for example here:
SuluHeadlessBundle/Content/ContentTypeResolver/SingleContactSelectionResolver.php
Lines 49 to 51 in 1bea2a1
if (null === $data) { | |
return new ContentView(null); | |
} |
but we dont need to do this in this pull request
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sorry i'm not sure anymore :D should i change something more ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no - i will create a PR for the other resolvers 🙂
Nice work - thanks a lot! |
See #3