Project

General

Profile

Actions

Feature #90994

closed

Add "active" class to current page to the default fluid_styled_content menu_pages, menu_sitemap_pages etc.

Added by Jonas Eberle about 4 years ago. Updated over 1 year ago.

Status:
Closed
Priority:
Should have
Assignee:
-
Category:
Fluid Styled Content
Target version:
-
Start date:
2020-04-10
Due date:
% Done:

100%

Estimated time:
PHP Version:
Tags:
menu
Complexity:
Sprint Focus:

Description

The data is already in the template, just the default templates would need to be changed.

Actions #1

Updated by Riccardo De Contardi about 4 years ago

I'd vote for

- put class on the <li> tag
- use separate classes for active and current states

[additional, opinionated]

would be nice maybe to have BEM classes, like

- .t3-menu on the <ul> tag
- .t3-menu__item on each menu item
- .t3-menu__item--active and .t3-menu__item--current for the states

[warning]

It is very likely that if you want to use a framework like Bootstrap or Foundation you'll have to do additional work in rewriting everything!

Framework menu ( <ul> ) class menu item ( <li> ) class anchor ( <a> ) class active state class current state class
Twitter Bootstrap nav nav-item nav-link active (on <a> ) none
Zurb Foundation menu none none is-active (on <li> ) none

This topic requires carefulness... honestly I'd prefer to keep TYPO3 on frontend agnostic from frameworks, and let the integrators use which one they prefer.

Actions #2

Updated by Jonas Eberle about 4 years ago

@Riccardo: Sorry, our research overlapped here ;) I didn't check for new posts before.

I checked these:
Foundation .vertical.menu on the <li> - https://get.foundation/sites/docs/menu.html#vertical-menu
Bootstrap 3 .nav-pills on the <li> - https://getbootstrap.com/docs/3.4/components/#nav
Bootstrap 4 nav components put it on the <a> but need additional markup anyways: https://getbootstrap.com/docs/4.4/components/navs/#vertical (why?...)

-- so I would also favor <li>

Undecided about BEM - useful yes, but very cluttery and should not be introduced for a single default content IMHO but if, then for all.

I would like to hear Benji about it, too.
I hope we'll talk about it in the next call of the rendering group of the structured content initiative.

This is how I currently integrate menu_sitemap_pages with Bootstrap 3:

// (scss)
.frame-type-menu_sitemap_pages {
    ul {
        @extend .nav;
        @extend .nav-pills;
        @extend .nav-stacked;
        list-style: none;
        padding-left: 0;

        ul {
            padding-left: 30px;
        }
    }
}

This covers the base already for a directory structure.
It produces some unnecessary CSS, but saves me from adapting default templates which I think is a big plus for maintainability.

Actions #3

Updated by Riccardo De Contardi about 3 years ago

Just to write something again on this topic: another nice suggestion is using the "namespaced CSS" convention

in this convention

- the menu should be a component so the prefix would be c- (IMHO, I would head some other opinions) or, as on my previous post, we could just imagine a t3- namespace
- the active state is just is-active (Foundation already follows it)

<ul class="c-menu">
   <li class="is-active">....</li>
</ul>

or if you prefer the anchor being "active":

<ul class="c-menu">
   <li><a class="is-active">....</a></li>
</ul>

some articles:

https://csswizardry.com/2015/03/more-transparent-ui-code-with-namespaces/
https://css-tricks.com/combining-the-powers-of-sem-and-bio-for-improving-css/

Actions #4

Updated by Benni Mack over 2 years ago

  • Status changed from New to Needs Feedback
  • Assignee deleted (Benjamin Kott)

How to proceed here? Jonas, do you want to create a patch?

Actions #5

Updated by Jonas Eberle over 2 years ago

Yes, I can do that.

Did we agree on a simple `<li class="is-active">` and integrators can go from there?

Actions #6

Updated by Benni Mack over 2 years ago

Jonas Eberle wrote in #note-5:

Yes, I can do that.

Did we agree on a simple `<li class="is-active">` and integrators can go from there?

Sure, why not...

Actions #7

Updated by Gerrit Code Review over 2 years ago

  • Status changed from Needs Feedback to Under Review

Patch set 1 for branch main of project Packages/TYPO3.CMS has been pushed to the review server.
It is available at https://review.typo3.org/c/Packages/TYPO3.CMS/+/73048

Actions #8

Updated by Gerrit Code Review over 2 years ago

Patch set 2 for branch main of project Packages/TYPO3.CMS has been pushed to the review server.
It is available at https://review.typo3.org/c/Packages/TYPO3.CMS/+/73048

Actions #9

Updated by Gerrit Code Review over 2 years ago

Patch set 3 for branch main of project Packages/TYPO3.CMS has been pushed to the review server.
It is available at https://review.typo3.org/c/Packages/TYPO3.CMS/+/73048

Actions #10

Updated by Michael Telgkamp over 2 years ago

The accessible way to mark the current page is adding `aria-current="page"` so I would like this to be used.

It can also be used to style the link using the selector [aria-current="page"]. It is important to have the class on the <a> Tag, not on the link, but that should not be a problem.

Actions #11

Updated by Jonas Eberle over 2 years ago

Thanks.
I am changing it to

  <li>
    <a aria-current="page">...</a>
  </li>
Actions #12

Updated by Jonas Eberle over 2 years ago

In order to allow styling of the <li> as well, would you prefer to add some CSS classes, too? (not an accessiblity question)

The problem being that we still do not have a selector like `li:has([aria-current=page])` as far as I know.

Like

<li class="is-active">
  <a aria-current="page">...</a>
</li>

And while we are at it, should we implement that for nested trees (used in the CE "Sitemap" for example) as well?

<ul>
  <li class="has-active">
    <a href=...>...</a>
    <ul>
      <li class="is-active">
        <a aria-current="page" href=...>...</a>
      </li>
    </ul>
  </li>
</ul>
Actions #13

Updated by Michael Telgkamp over 2 years ago

I would like to suggest to use active (or is-active ) as class name for an li in the rootline and current (or is-current ) as class for the li for the current page (to also keep it in line with the `aria-current` naming).

Actions #14

Updated by Jonas Eberle over 2 years ago

I agree.

Actions #15

Updated by Gerrit Code Review over 2 years ago

Patch set 4 for branch main of project Packages/TYPO3.CMS has been pushed to the review server.
It is available at https://review.typo3.org/c/Packages/TYPO3.CMS/+/73048

Actions #16

Updated by Gerrit Code Review over 2 years ago

Patch set 5 for branch main of project Packages/TYPO3.CMS has been pushed to the review server.
It is available at https://review.typo3.org/c/Packages/TYPO3.CMS/+/73048

Actions #17

Updated by Gerrit Code Review over 2 years ago

Patch set 6 for branch main of project Packages/TYPO3.CMS has been pushed to the review server.
It is available at https://review.typo3.org/c/Packages/TYPO3.CMS/+/73048

Actions #18

Updated by Gerrit Code Review over 2 years ago

Patch set 7 for branch main of project Packages/TYPO3.CMS has been pushed to the review server.
It is available at https://review.typo3.org/c/Packages/TYPO3.CMS/+/73048

Actions #19

Updated by Anonymous over 2 years ago

  • Status changed from Under Review to Resolved
  • % Done changed from 0 to 100
Actions #20

Updated by Benni Mack over 1 year ago

  • Status changed from Resolved to Closed
Actions

Also available in: Atom PDF