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 different field to be used as page label in list of pages #1122

Merged

Conversation

karfau
Copy link
Contributor

@karfau karfau commented Jun 1, 2017

From the documentation and my current understaning the title field is used display the page, and the menu field is used for navigational purposes.

When a content editor is navigating the pages in the admin plugin using the menu field also provides an easy option to give more useful information, e.g. in a long list of child pages.

I'm happy to do this change in more places where this concern might be present, but I know to little about the overall structure of the admin plugin to search by myself.

This could of course also be a configurable option, but as menu falls back to title it shouldn't do to much harm.
(I just had the idea of checking if the title is contained in menu and displaying both if not...)

Any feedback is welcome.

Copy link
Contributor

@flaviocopes flaviocopes left a comment

Choose a reason for hiding this comment

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

I'm not sure I agree on this change - menu is just used in the navigation (in the menu), while a page might also appear in other places, and the reason you might have "menu" different than the page title are purely presentation reasons, for example if the page title does not fit the layout.

That's my 2c, maybe other people have another opinion on the matter.

@OleVik
Copy link
Contributor

OleVik commented Jun 3, 2017

I agree with the case of long titles breaking the layout, but the more elegant solution there is to truncate it with CSS. menu is not a very common element within Grav, only specifically used where navigation is implemented. Reliance on it can break plugins and would be a superfluous property if it is just a shorter version of title.

@karfau
Copy link
Contributor Author

karfau commented Jun 4, 2017

So far I only care about the list of pages, where it looks to me like there is always plenty of space.
But when working with it during the day, I did see some places where it might be valuable. (e.g. list of sortable/unsortable pages)

I'm willing to invest into making it a config option for the admin plugin to use either title or menu or both in case there is enough space. (e.g. in the same way as admin.children_display_order)

But I will only invest that time when I have a clear signal from you, that this would be useful.

I'm also open for other ideas on how to customize what gets displayed in the admin panel.

Copy link
Member

@rhukster rhukster left a comment

Choose a reason for hiding this comment

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

I think an option in the plugin that allows you configure how pages are displayed is the best option. PR for this is welcome

@karfau karfau force-pushed the feature/use-menu-instead-of-title-in-page-list branch from 3b3cc0e to 1007547 Compare July 2, 2017 08:53
@karfau
Copy link
Contributor Author

karfau commented Jul 2, 2017

I added the option pages_list_display_field and changed my code to use it.
I wasn't sure where to configure the default value, looks like defined filter is not used a t all in the admin plugin.
Just realized I have some duplication in my code, display_field and page_label both make sure it defaults to title, one should be enough. Which one would you prefer?

@karfau karfau changed the title use menu instead of title in list of pages Allow different field to be used fas page label in list of pages Jul 2, 2017
@karfau karfau changed the title Allow different field to be used fas page label in list of pages Allow different field to be used as page label in list of pages Jul 2, 2017
@karfau
Copy link
Contributor Author

karfau commented Jul 2, 2017

One more remark, I only tested this code inside the stable version and cherry picked into this branch, as I have not set up a development environment with latest beta versions yet.

@OleVik
Copy link
Contributor

OleVik commented Jul 2, 2017

You really should clone and install from develop, as PRs are only relevant when developed and tested for code that will make it into the next release. Any conflict between stable and develop will propagate and have to be resolved before release, so using anything other than the develop-branch is counterproductive.

@karfau
Copy link
Contributor Author

karfau commented Jul 2, 2017

The branch that used for this PR is based on develop, and taking a look at the changes contained in that branch shows it does not contain anything but adding the configuration and using it.

But I'm going to create a dev environment, test the code in this one and report back.

@karfau
Copy link
Contributor Author

karfau commented Jul 2, 2017

Also works for me with the dev setup using all develop branches (except admin plugin, of course).
Both default value, using menu as value and non existing boo (fallback to title).

The only question open is the one I raised this morning:

Just realized I have some duplication in my code, display_field and page_label both make sure it defaults to title, one should be enough. Which one would you prefer?

@karfau
Copy link
Contributor Author

karfau commented Jul 2, 2017

I compared the two options and in my opinion the more readable/ easy to understand code is when the fallback is applied where the data is used.

@karfau
Copy link
Contributor Author

karfau commented Jul 9, 2017

Any feedback from your side @rhukster or changes required before this can land?

@rhukster rhukster merged commit 8170227 into getgrav:develop Jul 26, 2017
@rhukster
Copy link
Member

Accepted for next release! Thanks.

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.

4 participants