silverstripe / silverstripe-cms

Silverstripe CMS - this is a module for Silverstripe Framework rather than a standalone app. Use https://github.com/silverstripe/silverstripe-installer/ to set this up.
http://silverstripe.org/
BSD 3-Clause "New" or "Revised" License
516 stars 333 forks source link

Redirector page in Menu does not return expected isCurrent() value. #2905

Closed davejtoews closed 1 year ago

davejtoews commented 1 year ago

A common pattern for me in building SiteTrees is to add a Redirector Page under a page that has other sub pages so that the top level page can be used as a dropdown in my navigation, and then presented as a selectable option under itself.

RedirectorPage, inheriting isCurrent() (and isSection() for that matter) from SiteTree can not ever return true when iterating the menu because the currentPage will never have the ID of a redirector page.

For me, this means that the only viable option for navigating to some pages does not get the proper html class to be given an active state.

My use case is certainly not the only one, and I can imagine some people would not want Redirector page returning true for isCurrent() but I wonder if this has been considered or left this way by default. It seems to me my use case could be addressed by adding a checkbox w/in the CMS to opt-in to displaying as current, or by adding an extension point in SiteTree::isCurrent().

For the time being I have added the following to Page.php to get my intended results.

public function isCurrent()
{
    if (is_a($this, RedirectorPage::class)) {
        return $this->LinkTo()->isCurrent();
    }

    return parent::isCurrent();
}

public function isSection()
{
    if (is_a($this, RedirectorPage::class)) {
        return $this->LinkTo()->isSection();
    }

    return parent::isSection();
}
michalkleiner commented 1 year ago

Hi @davejtoews, thanks for opening the issue.

I'd say you need to handle this in your project code and as you are already doing, it's ok to do so e.g. in the Page class.

Redirector page is not meant to be able to be current or a section as it should just take you to the destination upon visiting it, so it's not a viewable page to stay on.

I would say you might be better off adjusting how you create your menus and duplicate the parent page as the first/last item (or whatever other logic you have there). You can also extend RedirectorPage to MenuRedirectorPage and put the isCurrent and isSection logic there and then replace it via Injector, or hide Redirector page type and only use your custom page type etc. There is multiple different ways how you can go about it individually, but I don't think this will turn into anything substantial within the CMS module itself.

With that, I'll close the issue for now. Thank you for your understanding.