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

Allow including of "parent" menu manifests #10548

Closed
wants to merge 2 commits into from

Conversation

Bakual
Copy link
Contributor

@Bakual Bakual commented May 18, 2016

When we have multiple layouts (menu item types) for the same component view, we more or less copy the whole manifest and just add or remove a few parameters from it. The rest is duplicated code.
This can be seen for example when looking at the manifests for the com_content category view and its two layouts (blog and list/default).
When we add a new feature to the view itself, we have to maintain the parameters in both layout manifests.

Summary of Changes

This PR makes use of the existing (probably not very well known) view and component manifest feature. Instead of duplicating the parameters in each layout manifest, we now can just specify to include the view and/or component manifest file and put the shared parameters into those manifests.
With this PR Joomla will now first load the layout manifest file (eg the default.xml one) and if it specifies to load one of the "parent" files it will load the view and/or the component one (metadata.xml).

When parameters are specified in multiple files, then the layout file takes precedence over the view file over the component file.
Also parameters specified in the "parent" files will appear after the parameters defined in the layout file.

This PR changes the menu item model to load those option files and moves the shared parameters from the com_content category view to the view manfiest file so we have a good test case 😄

Testing Instructions

  • Check the parameters in the category view of com_content (category list and category blog) and verify that all are still there after applying the PR
  • Check the functionality of the parameters, there should be no change at all.

Side Effects

When working on this PR I detected a few inconsistencies between the two layouts:

  • The blog layout has an additional option for the parameters in the "Options" tab called "Use Article Settings". That option was missing in the list layout. It's now available in both layouts.
  • The blog layout has two additional parameters in the "Options" tab called Position of Article Info and Show Tags. Those are used in the blog layout itself (and not in the list layout), however in the single article view they exist as well. We could move them to the view file as well but I left it for now since it is a good showcase for how it will work 😄

@memonica
Copy link

I succefully tested. #PBFIT


This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/10548.

@izharaazmi
Copy link
Contributor

What if we want to inherit all parameters but one from the parent xml? Is there anything possible than again doing it the old way?

@Bakual
Copy link
Contributor Author

Bakual commented Oct 20, 2016

I doubt this can be done easy. It would need at lot more fiddling with JForm than what is currently done.
It for sure is not in the scope of this PR. If accepted, the feature may be expanded afterwards in separate PRs.

@ghost
Copy link

ghost commented Jan 8, 2017

@Bakual is this still for testing?

@Bakual
Copy link
Contributor Author

Bakual commented Jan 8, 2017

Sure

@ghost
Copy link

ghost commented Jan 8, 2017

After "Apply Patch" got "Error: The file marked for modification does not exist: components/com_content/views/category/metadata.xml

@Bakual
Copy link
Contributor Author

Bakual commented Jan 8, 2017

Looks like I have to resolve some conflicts first. I'll try to do that tomorrow and will ping you.

@Bakual Bakual force-pushed the MenuIncludeParentManifest branch 3 times, most recently from e03e381 to 786d890 Compare January 8, 2017 19:51
@Bakual
Copy link
Contributor Author

Bakual commented Jan 8, 2017

@franz-wohlkoenig Conflicts are solved, it should work again.

@ghost
Copy link

ghost commented Jan 9, 2017

@Bakual Conflicts are solved.

Is in menue category list and category blog only tab Category or from tab Details to tab Module Assignment to test?

@Bakual
Copy link
Contributor Author

Bakual commented Jan 9, 2017

You need to check all the tabs between and including "Details" and "Integration".
The "Link Type" and the ones after that one are coming from a different source haven't changed.

@ghost
Copy link

ghost commented Jan 10, 2017

I have tested this item ✅ successfully on 786d890

Category List

  • Options: Show Associations shows now Use Global (Hide)
  • Use Article Settings not shown
  • Page Subheading before & after Patch not shown

Category Blog & List

Layouts: Partial different order inside Tabs (i.E. # Articles to List is now on top of second row)

As Points above are with and without Patch or Side Effects i chose " Tested sucecessfully".


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/10548.

@Bakual
Copy link
Contributor Author

Bakual commented Jan 10, 2017

The ordering can change when an option is only available in one layout. So that would be expected.
Your results look as expected I'd say.

@brianteeman
Copy link
Contributor

Can you look at resolving the conflicts so that this can be tested please.

@Bakual Bakual force-pushed the MenuIncludeParentManifest branch from 786d890 to eee64ed Compare August 20, 2017 19:33
@Bakual
Copy link
Contributor Author

Bakual commented Aug 20, 2017

Rebased and fixed conflicts.

@Bakual
Copy link
Contributor Author

Bakual commented Jan 8, 2018

I'm closing this PR because there seems to be little interest and resolving the conflicts is to risky by now.
If the interest arises for some reason again, it is easier to just rebuild the PR.

@Bakual Bakual closed this Jan 8, 2018
@Bakual Bakual deleted the MenuIncludeParentManifest branch January 8, 2018 14:47
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.

5 participants