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

FEATURE: Add fusion-prototypes Component, Editable, ContentComponent and Augmenter #1752

Merged
merged 9 commits into from Dec 5, 2017
Merged

FEATURE: Add fusion-prototypes Component, Editable, ContentComponent and Augmenter #1752

merged 9 commits into from Dec 5, 2017

Conversation

jonnitto
Copy link
Member

@jonnitto jonnitto commented Nov 13, 2017

The prototypes Component, Editable, ContentComponent and Augmenter are transfered from the package PackageFactory.AtomicFusion (https://github.com/PackageFactory/atomic-fusion) into the Neos-core.

  • Neos.Fusion:Component: Create a component that adds all properties to the props context and afterward evaluates the renderer.
  • Neos.Neos:Editable: Create an editable tag for a property. In the frontend, only the content of the property gets rendered.
  • Neos.Neos:ContentComponent: Base type to render component based content nodes, extends Neos.Fusion:Component
  • Neos.Fusion:Augmenter: Add html-attributes to renderer code as processor or as a standalone prototype.

In addition the class \Neos\Neos\Service\HtmlAugmenter was moved to \Neos\Fusion\Service\HtmlAugmenter with a deprecated backwards compatible layer.

@mficzel mficzel self-requested a review November 13, 2017 16:15
@mficzel mficzel changed the title TASK: Add Atomic Fusion FEATURE: Add fusion-prototypes Component, Editable, ContentComponent and Augmenter Nov 13, 2017
Copy link
Member

@mficzel mficzel left a comment

Choose a reason for hiding this comment

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

Looks good by reading

# The Augmenter-component can be used as processor or as a standalone prototype

prototype(Neos.Neos:Augmenter) < prototype(Neos.Fusion:Component) {
@class = 'PackageFactory\\AtomicFusion\\FusionObjects\\AugmenterImplementation'
Copy link
Contributor

Choose a reason for hiding this comment

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

this should be Neos\\Neos and should have been caught in a test.. make sure not to test this with the Atomic.Fusion package installed..

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed

@@ -0,0 +1,34 @@
# Create an editable tag for a property. In the frontend,
# only the content of the property gets rendered.

Copy link
Contributor

Choose a reason for hiding this comment

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

no need for two empty lines

Copy link
Member Author

Choose a reason for hiding this comment

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

Solved

property = null

# Decides if the editable tag should be a block element (`div`) or an inline element (`span`)
block = true
Copy link
Contributor

@aertmann aertmann Nov 13, 2017

Choose a reason for hiding this comment

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

What about h1 or p editables? This abstraction is limiting. To override you have to change renderer.editable.renderer.tagName which is pretty cumbersome. Instead maybe get rid of the abstraction and just accept tagName here.

Copy link
Member

Choose a reason for hiding this comment

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

@aertmann this is a design decision. By not allowing to control the tagName of the editable the rendering component has to take care of the meaningful markup. That way those components can perform well with editables passed or with data fetched vie flowQuery.

Copy link
Contributor

Choose a reason for hiding this comment

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

That makes sense, but does that justify the limitation? With Aloha editables like h1 and p behave differently as they don't allow any child tags inside them, however you can remove the h1 tag if it's rendered inside a div or span tag. This is useful when a property should be restricted to a specific wrapping tag. Thus not supporting this in an easy way is favoring being able to render editables without wrapping tags over being able to use text only editables. This is because with the view helper, there will always be a tag around the editable and to remove it you have to do conditional stuff to only use the editable view helper in editing mode.

Maybe there's a way to support the help users achieving both?

Copy link
Member

Choose a reason for hiding this comment

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

I personally think that this should be part of the editor-configuration and avoid editors behaving different if placed inside a specific tag. In the data is stored in a property of the node and might be rendered in different places.

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree, however that's not how it is currently. I can't speak for the new UI though, maybe someone knows if how you achieve text only editables of a specific tag and if it's different from how Aloha works.

Copy link
Member

Choose a reason for hiding this comment

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

@aertmann i discussed that with @grebaldi and we came to the conclusion that aloha and the ck-editor in the new ui both make a distinction between block and inline containers. This can be achieved with the current component interface already. We think we are fine here.

Copy link
Contributor

Choose a reason for hiding this comment

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

@mficzel okay thanks for checking up, guess you could argue that using a inline editable inside a h1 tag would do the trick, although not a easy as just setting the tag to h1

Copy link
Member

@mficzel mficzel left a comment

Choose a reason for hiding this comment

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

Looking good by reading and testing

@jonnitto
Copy link
Member Author

jonnitto commented Dec 5, 2017

@mficzel, thank you for your changes. It is looking good by reading and testing.

Copy link
Contributor

@grebaldi grebaldi left a comment

Choose a reason for hiding this comment

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

Looks fine (by reading)

@mficzel mficzel merged commit 89f322a into neos:master Dec 5, 2017
@mficzel mficzel moved this from Needs review to Done in Neos 3.3 & Flow 4.3 Release Board Dec 5, 2017
@jonnitto jonnitto deleted the task/addAtomicFusion branch December 5, 2017 09:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

None yet

5 participants