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: implements @private #158

Open
wants to merge 1 commit into
base: 8.3
Choose a base branch
from

Conversation

Jan3k3y
Copy link

@Jan3k3y Jan3k3y commented Mar 10, 2023

  • Use @Private in Headline and Slider
  • Refactor Slider renderer into afx
  • todo for refactor slider component

@mhsdesign
Copy link
Member

further todo would be to look at all usages of this

@Jan3k3y Jan3k3y force-pushed the task/privateProps branch 2 times, most recently from 0a49dae to 57197c6 Compare March 10, 2023 17:44
@ahaeslich ahaeslich mentioned this pull request Mar 24, 2023
@ahaeslich ahaeslich deleted the branch neos:8.3 April 1, 2023 15:15
@ahaeslich ahaeslich closed this Apr 1, 2023
@ahaeslich ahaeslich reopened this Apr 1, 2023
@ahaeslich ahaeslich changed the base branch from feature/new-content to 8.3 April 1, 2023 15:46
- @Private in Headline, Book and Slider
- rewrote Slider renderer into afx
- todo for refactor slider component
@mhsdesign mhsdesign marked this pull request as ready for review April 1, 2023 15:55
`
}

//todo rework this component, to verify what is used and/or needed
Copy link
Member

Choose a reason for hiding this comment

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

@mhsdesign and/or @Jan3k3y do you still want to work on this todo?

@@ -18,37 +19,36 @@ prototype(Neos.Demo:Presentation.Slider) < prototype(Neos.Fusion:Component) {
gap = 12
}

attributes = Neos.Fusion:DataStructure
Copy link
Member

Choose a reason for hiding this comment

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

Did you notice that with removing this and the following changes for attributes you can't add additional attributes to the slider anymore? So this refactoring would change the components behaviour.

Copy link
Member

Choose a reason for hiding this comment

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

I wouldn't change the interface in such a PR

Copy link
Member

Choose a reason for hiding this comment

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

we actually dont need the attributes ourselves (for the integration)

so this was only exposed so possibly other people could use the slider component themselves?

This seems like an odd concept - as for me the neos demo is just a demo without any contract and any expectations any guarantees -> Otherwise our refactor and redesign from 8.2 to 8.3 would have been breaking and illegal.

So i disagree. Imo we are allowed to change it, and in our case it makes the component easier to maintain and migrate to private props.

@markusguenther
Copy link
Member

Guess we did not finish this for the release of 8.3.0 but would be a nice update afterwards :)

@@ -18,37 +19,36 @@ prototype(Neos.Demo:Presentation.Slider) < prototype(Neos.Fusion:Component) {
gap = 12
}

attributes = Neos.Fusion:DataStructure
Copy link
Member

Choose a reason for hiding this comment

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

I wouldn't change the interface in such a PR

@crydotsnake
Copy link
Member

Whats the status here?

@mhsdesign
Copy link
Member

feel free for anyone to pick up this work ;)

@ahaeslich
Copy link
Member

@Jan3k3y & @mhsdesign do you want to finish this and add an @private example for the final Neos 9 release? 😇

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

Successfully merging this pull request may close these issues.

7 participants