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

TASK: Rewrite Neos.Neos:Shortcut to pure fusion #3372

Merged
merged 10 commits into from
Nov 1, 2023

Conversation

mhsdesign
Copy link
Member

@mhsdesign mhsdesign commented Jul 2, 2021

The backend rendering for shortcut nodes is reimplemented in the fusion prototype Neos.Neos:Shortcut and the previous fluid template is removed.

resolves: #3261

I tested all possible backend views of the Neos.Neos:Shortcut in the Neos.Demo:

  • shortcut to the parent page
  • shortcut to image
  • shortcut to external URI
  • shortcut to specific node target / reference
  • shortcut to first child page

Copy link
Member

@daniellienert daniellienert left a comment

Choose a reason for hiding this comment

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

Hey @mhsdesign,

thank you very much for working on that task!!!

I think we can shrink the fusion down and make it more readable, see my comments. Currently fusion only renders in frontend language, so we should discuss, if we should add a backendTranslationHelper instead of a BackendUserHelper.

Copy link
Member Author

@mhsdesign mhsdesign left a comment

Choose a reason for hiding this comment

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

okay thanks for your advice ;)

i think a BackendTranslationHelper might be better too. I will work on it ;)

@mhsdesign mhsdesign force-pushed the FusionRewrite-NeosNeosShortcut branch from 980e5f5 to 8a6bf63 Compare September 14, 2021 21:20
@mhsdesign mhsdesign changed the title Rewrite Neos.Neos:Shortcut to pure fusion + BackendUserHelper Rewrite Neos.Neos:Shortcut to pure fusion + BackendTranslationHelper Sep 17, 2021
@mhsdesign
Copy link
Member Author

mhsdesign commented Sep 17, 2021

Some points left - wich need to be discussed:

@mhsdesign
Copy link
Member Author

oops... crowdin is no more? #3437 (comment)

@markusguenther markusguenther force-pushed the FusionRewrite-NeosNeosShortcut branch from 8a6bf63 to cbf2519 Compare October 10, 2021 18:48
@mhsdesign
Copy link
Member Author

mhsdesign commented Oct 11, 2021

since im extending the flow translation helper and modifing a protecteded method - this will fail silently if the flow guys rename the method. Should i implement a method_exists check in the constructor? I guess the best would be to refactor the flow translation helper to make it easily extensible.

@mhsdesign mhsdesign requested a review from bwaidelich November 24, 2021 23:24
@mhsdesign
Copy link
Member Author

mhsdesign commented Feb 8, 2022

So i thought about this again, the fluid <neos:backend.translate> implementation:
https://github.com/neos/neos-development-collection/blob/ddf0b3f273395cb4dee316c5d0205b603abba5f7/Neos.Neos/Classes/ViewHelpers/Backend/TranslateViewHelper.php uses also inheritance and some ugly duplicated code ... id like to avoid that, but there is no simple way around that.

Similar to the fluid view helper, it would look for the eel helper like this (by @jonnitto ):
https://github.com/CarbonPackages/Carbon.Eel/blob/b2cf962d4dc367f79d41d573cc9766223ed0787a/Classes/EelHelper/BackendHelper.php

I just see inheritance in this case as not a good design choice.

@mhsdesign mhsdesign dismissed daniellienert’s stale review February 12, 2022 20:08

code has been adjusted

@mficzel mficzel self-requested a review March 15, 2022 09:39
@mhsdesign mhsdesign marked this pull request as draft March 16, 2022 12:13
@mhsdesign mhsdesign changed the title Rewrite Neos.Neos:Shortcut to pure fusion + BackendTranslationHelper WIP: Rewrite Neos.Neos:Shortcut to pure fusion Mar 16, 2022
@mhsdesign
Copy link
Member Author

this is WIP, until #3638 is solved.

@mhsdesign mhsdesign added the Stale Issues and PRs with no recent activity, about to be closed soon. label Oct 25, 2022
with neos#3937 we dont need the fallbacks anymore ;)
Otherwise, changes from `Neos.Backend.interfaceLanguage()` will not be reflected.

And its like a backend module -> which i wouldn't cache also.
@mhsdesign mhsdesign force-pushed the FusionRewrite-NeosNeosShortcut branch from 4f444ee to 1ce5b15 Compare November 2, 2022 12:42
@mhsdesign mhsdesign removed I: WIP Stale Issues and PRs with no recent activity, about to be closed soon. labels Nov 2, 2022
@mhsdesign mhsdesign changed the title WIP: Rewrite Neos.Neos:Shortcut to pure fusion Rewrite Neos.Neos:Shortcut to pure fusion Nov 2, 2022
@mhsdesign mhsdesign marked this pull request as ready for review November 2, 2022 12:43
@mhsdesign
Copy link
Member Author

With #3937 (waiting for upmerge) and #3371 in place, the path of my first Neos PR can continue after beeing 14 months stale! ;)

Open for reviews ;)

@mhsdesign
Copy link
Member Author

Okay, #3937 is now upmerged (and available in this branch), so now it's really ready for review ;)

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.

Works as expected and and removes the fluid template.

  1. The pr should start with a brief description that is suitable for the release notes like:

The backend rendering for shortcut nodes was is reimplemented via the new fusion prototype Neos.Neos:Shortcut and the previous fluid template is removed.

  1. While this is pretty much an 1:1 fusion implementation. i personally would prefer to use some prototypes inside to make this extensible but consider this optional.
    It could work with a central Neos.Neos:Shortcut.Case that does the switching between Neos.Neos:Shortcut.ParentNode, Neos.Neos:Shortcut.FirstChildNode and Neos.Neos:Shortcut.SelectedTargetNode

@mhsdesign mhsdesign changed the title Rewrite Neos.Neos:Shortcut to pure fusion Task: Rewrite Neos.Neos:Shortcut to pure fusion Nov 4, 2022
@jonnitto jonnitto changed the title Task: Rewrite Neos.Neos:Shortcut to pure fusion TASK: Rewrite Neos.Neos:Shortcut to pure fusion Nov 11, 2022
@mhsdesign
Copy link
Member Author

2. While this is pretty much an 1:1 fusion implementation. i personally would prefer to use some prototypes inside to make this extensible but consider this optional.
It could work with a central Neos.Neos:Shortcut.Case that does the switching between Neos.Neos:Shortcut.ParentNode, Neos.Neos:Shortcut.FirstChildNode and Neos.Neos:Shortcut.SelectedTargetNode

good idea (WIP at my pc) but ill wait for #3943 ;)

also those parts need to be discussed #3372 (comment)

@bwaidelich
Copy link
Member

Thanks for this!

I agree to @mficzel's comments. Splitting this into smaller chunks would greatly enhance extensibility and readability.

good idea (WIP at my pc) but ill wait for #3943 ;)

Can we maybe get this in based on the current feature set? IMO #3943 is a great direction but it's a higher-risk-feature and it would be a pitty if it blocked this one.

Regarding

i found some css [...] should i move it here then? yes -> but how without breaking things

What about targeting 9.0 with this feature?

i disabled the cache for the backend shortcut view - as it previously didnt
calculate the current user language into the cache

Can't we add the user language to the @cache config?

@bwaidelich
Copy link
Member

It's been a while.. Is this one still in progress?

@mhsdesign mhsdesign dismissed jonnitto’s stale review October 31, 2023 08:20

Adjusted as requested

@mhsdesign
Copy link
Member Author

Neos ui part neos/neos-ui#3657

@mhsdesign mhsdesign changed the base branch from 8.2 to 9.0 October 31, 2023 09:39
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 and it works

@ahaeslich ahaeslich merged commit 64d2af1 into neos:9.0 Nov 1, 2023
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Archived in project
Status: In progress, but not this time ...
Development

Successfully merging this pull request may close these issues.

Rewrite Neos.Neos:Shortcut to pure fusion
6 participants