-
-
Notifications
You must be signed in to change notification settings - Fork 135
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 ButtonEditor as new editor #3475
base: 9.0
Are you sure you want to change the base?
Conversation
90b380f
to
e0a3cf9
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.
Hi @reflexxion,
thanks a lot for adding this editor! I really like the idea. I reviewed the code and added a couple of comments.
I also did some manual tests with the new editor and like to discuss some general observations:
What I'm writing here does not necessarily constitute actionable items within the scope of this PR. I'm just thinking out loud (by quietly typing words in a text box :D).
Defaults
For the type: string
case, I had to configure multiple: false
manually during my test. I think it should rather be the default. Likewise, I had to configure allowEmpty: true
manually, which I would have expected to be the default intuitively (though I believe there are probably many opinions here).
Editor defaults are still being configured in Neos.Neos
(see: https://github.com/neos/neos-development-collection/blob/93c5b77411b527fb99924b3eb8025e3653cdf4cb/Neos.Neos/Configuration/Settings.yaml#L223). We should probably change that.
And there's another conceptual problem here: We can configure defaults for a specific editor and we can configure defaults for a specific dataType. We can't configure (yet) a default for the combination of both, which would become relevant in this case.
Labels
If buttons are configured with a magic i18n
label, those labels are not covered by the automatic translation id mechanism. This is due to missing editor specifics in the NodeTypeConfigurationEnrichmentAspect
over at Neos.Neos
(see: https://github.com/neos/neos-development-collection/blob/93c5b77411b527fb99924b3eb8025e3653cdf4cb/Neos.Neos/Classes/Aspects/NodeTypeConfigurationEnrichmentAspect.php#L167).
I think it is high time the NodeTypeConfigurationEnrichmentAspect
moves over to Neos.Neos.Ui
.
Dirty-State indication
As of right now, the editor lacks dirty-state indication. When a value changes in the inspector it is important for the user to have visual feedback telling them, which value has changed.
Inspector Editors have to provide their own styling for this state. Convention is to have an orange outline around the most representative part of the editor. (see also #3504, which came up recently)
if (options.multiple && this.initialValueType === 'string') { | ||
console.warn(`Misconfiguration in property "${props.identifier}". Multiple is activated but value type seems to be "string" but should be "array".`); | ||
} |
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.
Imho this can be more aggressive. The editor should probably actually render this error message instead of the buttons :)
Though for the constraint check, it should not rely on analyzing the actual type of the value, but the configured type of the property (since it states "Misconfiguration").
An erroneous type of the property value should be handled gracefully by the editor.
AFAIK, the UI does not provide the configured type to the editors, but we should definitely change that. So hang on for a PR 😄, I'll see what I can do.
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.
Seems to go into the direction of #3580?
@grebaldi i actually like to discuss the naming of the new editor. When i read "button editor" in the pr title i didnt really understand what the editor might do until i saw the screenshots. (I imagined 1 button which can be clicked or edited idk it didnt make sense in my head ^^) maybe |
Look what I've put in the packages readme :-D
Yeah, I also wasn't really convinced by Probably something like @grebaldi thanks for your review. I'll update the PR soon. |
I like |
Yeah, you're right. I this change isn't all that necessary. We can change this later on :) |
@Sebobo proposed but we imo should avoid any naming confusion with our selectbox editor |
Add new
Neos.Neos/Inspector/Editors/ButtonEditor
as described in #3474