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

Make parent-template available in schema + pass it all the way to rou… #5337

Open
wants to merge 5 commits into
base: 2.5
Choose a base branch
from

Conversation

cirykpopeye
Copy link

@cirykpopeye cirykpopeye commented Jun 8, 2020

Q A
Bug fix? no
New feature? yes
BC breaks? no
Deprecations? no
Fixed tickets fixes #4922
Related issues/PRs #4922
License MIT
Documentation PR sulu/sulu-docs#479

What's in this PR?

Option to provide default template to children of a parent page.

Why?

An artists page should have artist as a default template, makes a lot more sense than always providing a default "Default".

To Do

  • Set the page template from the route attributes
  • call store.setType to set the template select value

@cirykpopeye
Copy link
Author

@danrot DefaultTemplate is now passed inside the form route attributes, if you have time could you have a look at how to reach the template dropdown?
This is the associated line where the type is set: https://github.com/sulu/sulu/blob/master/src/Sulu/Bundle/AdminBundle/Resources/js/containers/Form/stores/ResourceFormStore.js#L69

@niklasnatter
Copy link
Contributor

niklasnatter commented Jun 9, 2020

@cirykpopeye I just had a quick look at the code and it looks like the default templates are not needed in the frontend (anymore) to me 🙈

I think you can add the parentId to the $routerAttributesToFormMetdata of the PageAdmin and then access it in the getMetadata of the StructureFormMetadataLoader. That would be similar to how the webspace is handled. But maybe @danrot already had another idea/plan? 🙂

@jordygroote
Copy link
Contributor

Thanks @cirykpopeye for picking this up again. When having a rather large website with a few templates this creates confusing with our customers :)
Schermafbeelding 2020-06-10 om 09 25 41

@danrot
Copy link
Contributor

danrot commented Jun 10, 2020

@nnatter Actually @cirykpopeye has already suggested that to me, and I guess that would work. The reason I would like to try to avoid that, is that if we add the parentTemplate to the metadata request, the cache in the metadataStore wouldn't be as effective anymore. If you have 5 different templates, you might end up loading the same information about all templates up to 5 times in a single session. Therefore I would suggest to implement that part in JS.

@danrot
Copy link
Contributor

danrot commented Jun 10, 2020

The only thing I could imagine is that we return a defaultType for a generic default type and a defaultTypeMapping (not happy with the name, hopefully somebody has a better suggestion) in the metadata response. This way we would not have to load information more often than necessary, while still being able to reuse that functionality for other entities including types (although we don't have any yet, but somebody might have added custom entities, which could make use of that).

That would still mean some adjustments in the JS part, namely in the ResourceFormStore. Then we would have to pass the parentType instead of the already resolved defaultType to the ResourceFormStore somehow to achieve it. In order to make that easily reusable for other entities as well we would have to introduce a ListViewBuilder::addParentPropertiesToEditView, but that would not have to be done immediately, because we can currently implement that in the PageList (the only place it is currently occuring).

Alternatively we could follow up on the approach to implement getting the defaultType in the PageList, but then this logic is not available in other entities.

@cirykpopeye
Copy link
Author

The only thing I could imagine is that we return a defaultType for a generic default type and a defaultTypeMapping (not happy with the name, hopefully somebody has a better suggestion) in the metadata response. This way we would not have to load information more often than necessary, while still being able to reuse that functionality for other entities including types (although we don't have any yet, but somebody might have added custom entities, which could make use of that).

That would still mean some adjustments in the JS part, namely in the ResourceFormStore. Then we would have to pass the parentType instead of the already resolved defaultType to the ResourceFormStore somehow to achieve it. In order to make that easily reusable for other entities as well we would have to introduce a ListViewBuilder::addParentPropertiesToEditView, but that would not have to be done immediately, because we can currently implement that in the PageList (the only place it is currently occuring).

Alternatively we could follow up on the approach to implement getting the defaultType in the PageList, but then this logic is not available in other entities.

@danrot I don't directly see another use case for the default template instead of in pages or am I wrong?

@danrot
Copy link
Contributor

danrot commented Jun 16, 2020

@cirykpopeye Currently there is no other use case in the core, but I don't want to exclude that possibility for anything custom built, therefore I would probably go with the approach also adding it to the metadata response.

@sulu/core-team WDYT?

@chirimoya chirimoya requested a review from danrot June 18, 2020 17:31
@cirykpopeye
Copy link
Author

cirykpopeye commented Jun 24, 2020

@cirykpopeye Currently there is no other use case in the core, but I don't want to exclude that possibility for anything custom built, therefore I would probably go with the approach also adding it to the metadata response.

@sulu/core-team WDYT?

I now added the defaultTemplate to the metadata request, it now returns that template if it exists and the page template is set. Open for comments :)

@@ -159,7 +159,7 @@ public function configureViews(ViewCollection $viewCollection): void
];

$routerAttributesToFormRequest = ['parentId', 'webspace'];
$routerAttributesToFormMetdata = ['webspace'];
$routerAttributesToFormMetdata = ['webspace', 'defaultTemplate'];
Copy link
Contributor

Choose a reason for hiding this comment

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

This will fragment the cache far more than it is necessary, that is why I suggested in my comment to implement this in a different way. Here we will have a metadata request for every defaultTemplate that is configured, whereby with the other solution it would always be just a single metadata request.

@jordygroote
Copy link
Contributor

Any updates on this? We'd still love this functionality :)

@danrot
Copy link
Contributor

danrot commented Aug 25, 2020

@jordygroote Not that I am aware of, since I haven't heard of @cirykpopeye since my last review, and I didn't have any time to dig into this... Would you like to take over? 🙂

@cirykpopeye
Copy link
Author

cirykpopeye commented Aug 25, 2020

@jordygroote @danrot I have no idea how to implement it Danrot's way, I'd still like to complete this as we're in need of this feature also but atm I can't research on how to do this due to lack of time. React is not really my area

@danrot
Copy link
Contributor

danrot commented Aug 28, 2020

The change I've suggested is that the metadata always returns the same response, so that we don't have to add a query parameter to the URL. As explained above, the problem is that we would have to make the request for every single template then, while we currently have only one of those request during a user's session.

That could be achieved by returning a defaultTypeMapping or maybe defaultTypeByParentType attribute, which is a key-value representation, whereby the key is the parent type and the value is the default for this specific type. The parent template would then also have to be passed to the ResourceFormStore (probably as an optional argument in the constructor), so that we can access the value from the defaultTypeByParentType attribute in its handleSchemaTypeResponse method, probably somehow like this:

    @action handleSchemaTypeResponse = (schemaTypes: ?SchemaTypes) => {
        const {
            types = {},
            defaultType,
            defaultTypeByParentType
        } = schemaTypes || {};

        this.types = types;
        this.typesLoading = false;

        if (this.hasTypes) {
            // this will set the correct type from the server response after it has been loaded
            when(
                () => !this.resourceStore.loading,
                (): void => {
                    this.setType(this.resourceStore.data[TYPE] || defaultTypeByParentType(this.parentType) || defaultType || Object.keys(this.types)[0]);
                }
            );
        }

Mind the two appearances of defaultTypeByParentType.

@alexander-schranz alexander-schranz added the UX Affecting the end user label Jan 7, 2021
@alexander-schranz alexander-schranz changed the base branch from master to 2.x January 7, 2021 15:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
UX Affecting the end user
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Default template based on the template of the parent page
5 participants