-
-
Notifications
You must be signed in to change notification settings - Fork 224
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
FEATURE: Custom label for auto created child nodes #530
Conversation
By analyzing the blame information on this pull request, we identified @radmiraal, @skurfuerst and @aertmann to be potential reviewers |
576f54c
to
96314fc
Compare
Pretty useful for ContentCollection to replace "Content Collection (footer)", by something more human readable like "Document Footer". Also useful with complex node type with multiple child node, like a Staff, with multiple PostalAddress and ContactInformation, ...) |
@@ -71,13 +74,27 @@ public function initializeObject() | |||
*/ | |||
public function getLabel(NodeInterface $node, $crop = true) | |||
{ | |||
$label = \TYPO3\Eel\Utility::evaluateEelExpression($this->getExpression(), $this->eelEvaluator, array('node' => $node), $this->defaultContextConfiguration); | |||
$label = null; | |||
if ($node->getNodeType()->isOfType('TYPO3.Neos:ContentCollection') && $node->isAutoCreated()) { |
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.
This is a no-go as it introduces a semi dependency of Neos in CR. If we want to introduce this kind of feature it must work for all node types.
I could accept that and I can see how nice this can look like, but I am not 100% sold for this yet...
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 agree, but why does it even need to only be for content collections? isn't the fact that it's auto-created and has the label property set enough?
See inline comment 👎 for now |
$parentNode = $node->getParent(); | ||
$property = 'childNodes.' . $node->getName() . '.label'; | ||
if ($parentNode->getNodeType()->hasConfiguration($property)) { | ||
$expression = $parentNode->getNodeType()->getConfiguration($property); |
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'd prefer if this also accepted simple strings and not only expressions, since that's unnecessary most of the time
Nice idea, I think we can make it work.. see inline comments It's missing documentation as well though |
@aertmann @kitsunet Now support string, and no dependency to the ContentCollection anymore, it's pretty generic and useful. As discussed with @kitsunet last week, we need to take to not have too much configuration that can depend on the "context" (parent node, ...) or we will have hard time to debug it. Next step a small documentation and this one is merge ready |
Up to date with master and update the first PR comment to show the possibility to use a string or EEL expression as label. |
👍 Shoudl be fine! |
@aertmann If you can review again we can merge this one |
@@ -42,6 +42,7 @@ additionalProperties: | |||
type: dictionary | |||
additionalProperties: FALSE | |||
properties: | |||
'label': { type: string, description: "A string or an EEL expression for generating a node's of this node type's label. Alternatively an array with the key generatorClass and optional options." } |
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.
maybe more specific "and optional generator options."
*/ | ||
public function getLabel(NodeInterface $node) | ||
{ | ||
$label = Utility::evaluateEelExpression($this->getExpression(), $this->eelEvaluator, array('node' => $node), $this->defaultContextConfiguration); | ||
$label = null; | ||
if ($node->getNodeType()->isOfType('TYPO3.Neos:ContentCollection') && $node->isAutoCreated()) { |
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.
why this limitation on only ContentCollection?
Thanks @aertmann for the review, all comments fixed |
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 re-read the code and from a user perspective this is great, but I am still not convinced about the implementation. That's a lot of logic in the LabelGenerator that any person implementing the interface would have to replicate. Also The LabelGenerator shouldn't be in charge of figuring this out. If we want to do this, we might want to change the interface to tell the generator where to get the config from maybe. |
@kitsunet What if we introduce an Abstract class with helper function, and a protected method to override the label for the automaticaly created child nodes ? Like this custom implementation can reuse the abstract class and we avoid changing the interface ? |
And maybe a missing feature is the support of i18n keywords to have translatable labels ... |
I would like to try to bring this into 8.4 as recently I was sad again in two projects that the feature doesn't exist yet and I had to introduce ContentCollection subtypes for no further reason than to have proper labels. |
2a9c67e
to
4a06cce
Compare
4a06cce
to
3b9ec37
Compare
This change add a node type configuration to generate a custom node label for auto created child nodes: ‚Neos.NodeTypes:Page': superTypes: ‚Neos.Neos:Document': TRUE childNodes: main: label: ‚Main Content Collection' type: ‚Neos.Neos:ContentCollection' This allow to have a less technical content tree. This change include the schema validation and default label for default node types.
3b9ec37
to
2e7c49e
Compare
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.
Haha nice thanks for the restoration of this pr ^^
Would be great if you could create an upmerged version of this for Neos 9 as well (there we have different tests and the class was moved and the api changed ...)
@Sebobo This change has not been upmerged to Neos 9, as this will not work there anymore. Please provide a dedicated PR to 9.0 if this feature is also needed there. |
Resolves: #2276
This change add a node type configuration to generate a custom node label
for auto created child nodes:
This allow to have a less technical content tree. This change include the
schema validation and default label for default node types.
The label can be a string or an EEL expression. Inside the
expression you can access the parent node in the variable
parentNode
.