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: CKE5 placeholder insert plugin #46

Merged
merged 2 commits into from
Nov 13, 2019
Merged

Conversation

dimaip
Copy link
Contributor

@dimaip dimaip commented Aug 27, 2019

Requires: neos/neos-ui#2546

Adds a dropdown to CKE5 that allows you to insert Fluid placeholders for elements configured in the current form. It takes either formElement.properties.identifier or formElement.identifier as the placeholder value.

Sample config to enable this feature:

'Neos.Form.Builder:ConfirmationFinisher':
  properties:
    'message':
      ui:
        inspector:
          editor: 'Neos.Neos/Inspector/Editors/RichTextEditor'
          editorOptions:
            formatting:
              placeholderInsert: true
              strong: true
              em: true

image (2)

@dimaip dimaip requested a review from bwaidelich August 27, 2019 14:14

const placeholderLabel = this.props.i18nRegistry.translate(
"Neos.Form.Builder:Main:placeholder",
"Insert placeholder"

Choose a reason for hiding this comment

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

This should be localized.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is, once the translations will be in place, it would show up, this is just a fallback

const options = elementsNode.children
.map(node => this.props.nodesByContextPath[node.contextPath])
.map(node => ({
value: node.properties.identifier || node.identifier,

Choose a reason for hiding this comment

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

Would it be possible to have this in Settings? While thinking of it, I see that different use-cases are possible; sometimes you want to use speaking identifiers, sometimes not.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Could you describe your idea in more details: setting name, setting format, what exactly it does etc?

Choose a reason for hiding this comment

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

After thinking again, let's not over-complicate things for the moment and go with it as it is.

Copy link
Member

@markusguenther markusguenther 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 to me, just wondering with the version constraint to the @neos-project/neos-ui-extensibility.

"watch": "neos-react-scripts watch"
},
"devDependencies": {
"@neos-project/neos-ui-extensibility": "^1.3"
Copy link
Member

Choose a reason for hiding this comment

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

really version 1.3?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It doesn't matter really, it would compile with any version of the extensibility package. Will change to wildcard to avoid confusion.

@lorenzulrich
Copy link

We need to rethink if and when to add this to Form Builder:

  • Form Builder currently supports Neos 3.1+ and the old UI.
  • As far as I know, the old UI is also supported in Neos 4.3, even though I'm not sure if anyone still uses it. If we can remove support for the old UI, we could have a release targeting Neos 4.3+ without support for the old UI.
  • The underlying change in the Neos UI, FEATURE: CKE5 secondary inspector neos-ui#2546, is only targeting Neos 5. So we would need to have a separate version/branch with Neos 5 only support.

@lorenzulrich
Copy link

@dimaip just pointed out that this will not break anything even in older versions because it only works if the new UI is enabled and the UI has a RichTextEditor. Therefore, we could merge this without any drawbacks.

Copy link
Member

@bwaidelich bwaidelich left a comment

Choose a reason for hiding this comment

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

Thanks a lot for this great enhancement!

@bwaidelich bwaidelich merged commit 33ca630 into master Nov 13, 2019
@bwaidelich bwaidelich deleted the dimaip-placeholderInsert branch June 16, 2020 13:44
@paavo
Copy link
Contributor

paavo commented Aug 5, 2020

Thanks very much @dimaip for this awesome Feature.
Is there any hint, how to make this work with the Neos.Form.Builder:EmailFinisher too?
Looks like the "Frontend Part" works well, but the Values are not available when sending the E-Mail 😢

The "missing Property Selector" is still a reason, most of our Editors are not able to create their own Forms.
Editors are not able to edit HTML in the ConfirmationFinisher / EmailFinisher.
If this works, im pretty sure our Editors would be very happy 🎉

@dimaip
Copy link
Contributor Author

dimaip commented Aug 7, 2020

Ohh @paavo sorry I totally forgot everything I did for this feature, so can't answer it from top of my head.
Maybe somebody who actually uses this feature could help? @lorenzulrich? @bwaidelich?

@bwaidelich
Copy link
Member

Maybe somebody who actually uses this feature could help?

I don't use this feature to be honest, but I tried to help in the #guild-form-builder channel: https://neos-project.slack.com/archives/CH86S151D

TL;DR This change prepares the RichTextEditor for Confirmation and EmailFinisher but the latter expects form value placeholders in the form formValues.* so that can't work. neos/form#122 solves this by making the prefix optional..

BTW: The prefix was there to prevent naming conflicts with options for the finisher. An alternative solution might be to encourage the same prefix in the ConfirmationFinisher and to extend the RichTextEditor by some "placeholderPrefix" option

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

Successfully merging this pull request may close these issues.

5 participants