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 the user to show or hide folder pages in the menu #11

Merged
merged 4 commits into from Mar 5, 2018

Conversation

fritzmg
Copy link
Sponsor Contributor

@fritzmg fritzmg commented Dec 17, 2017

Imho it's better to let the user decide whether a folder page should be shown in the menu or not. See also #8 (comment)

However this way the user can 'accidentally' show an URL for a folder page in the front end, if the used navigation template is not based on the nav_default template of this commit.

I could add a generateFrontendUrl hook that simply creates \Contao\Environment::get('url') (i.e. scheme and host) for example. I am not sure that's worth it though.

@fritzmg
Copy link
Sponsor Contributor Author

fritzmg commented Jan 15, 2018

@aschempp @Toflar any objections to this?

@Toflar
Copy link
Member

Toflar commented Jan 16, 2018

I don't see why a folder page should ever be shown in the navigation. It's purpose is to explicitly not do that. /cc @qzminski

@fritzmg
Copy link
Sponsor Contributor Author

fritzmg commented Jan 16, 2018

The purpose would be to create navigation items that aren't pages themselves, e.g. parents for dropdowns, without having to resort to doing that with CSS classes for example. The original m17folderpage extension followed the same logic.

@aschempp
Copy link
Member

You should use the forward page for that. It offers exactly the same functionality as the folderpage, but you can define the target if the navigation (accidentally) is a link.

@qzminski
Copy link
Member

Although it is not a part of the initial concept I think it does not harm to have it. However this pull request is a BC break I fear, so it should be improved.

@aschempp that does not help because some forward pages may be used as a non-link whereas some as regular link. Folder page type would explicitly always work as a non-link.

@fritzmg
Copy link
Sponsor Contributor Author

fritzmg commented Jan 16, 2018

@aschempp no, because then you cannot have menu items that are an actual redirect, if you change your template for internal redirects to be <span> instead of <a>.

// @qzminski exactly :)

@fritzmg
Copy link
Sponsor Contributor Author

fritzmg commented Jan 16, 2018

@qzminski it could be released as 3.0.0, unless you have specific plans for 2.x

But why do you think it's a BC break anyway? This would only affect newly created folder pages, not existing ones.

@aschempp
Copy link
Member

@qzminski it would probably work because hide is already set on all existing pages (or at least it should be).

@aschempp
Copy link
Member

However, we must not override the nav_default template. Can you rename that to nav_folderpage or thelike?

@fritzmg
Copy link
Sponsor Contributor Author

fritzmg commented Jan 17, 2018

Sure. Also I think I should add the cssID field.

@qzminski
Copy link
Member

@aschempp doesn't hide=1 mean that the page will not be shown in the navigation? If we take it down it in this PR, then suddenly all websites using this extension will get an extra navigation point.

@fritzmg
Copy link
Sponsor Contributor Author

fritzmg commented Jan 17, 2018

@aschempp doesn't hide=1 mean that the page will not be shown in the navigation? If we take it down it in this PR, then suddenly all websites using this extension will get an extra navigation point.

No they won't because only the default value changes. This does not affect existing folder pages in your database.

@qzminski
Copy link
Member

Ah sorry I didn't notice that it is done when the page is being updated 🙈 All good then!

@aschempp
Copy link
Member

aschempp commented Feb 5, 2018

ping @fritzmg

@fritzmg
Copy link
Sponsor Contributor Author

fritzmg commented Feb 5, 2018

Done. I have made the following changes:

  • 959afe5: added the cssClass field to the palette, since the folder page could now be visible in the main menu and thus you might want to assign a CSS class for that menu item.
  • 0c53aeb: if you create a regular page, save it with an alias and then change it to a folder page, the alias will remain. The existing save_callback will not fix this, since this callback is only executed if the field actually exists in the palette. May be this callback can be removed in general?
  • 5ac5961: renamed the nav_default template to nav_folderpage.

@fritzmg
Copy link
Sponsor Contributor Author

fritzmg commented Mar 1, 2018

Any further comments?

@qzminski
Copy link
Member

qzminski commented Mar 1, 2018

RTM to me. @terminal42/pilots ?

@Toflar
Copy link
Member

Toflar commented Mar 1, 2018

No idea, that's not my baby. Feel free if it's okay to you :D

@qzminski
Copy link
Member

qzminski commented Mar 1, 2018

@aschempp your call then.

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

Successfully merging this pull request may close these issues.

None yet

4 participants