1) Style for list items do not match drupal.css

In 4.7 drupal.css div.menu has been changed to ul.menu, so the styles these blocks had are now gone.

2) When a term has nodes, but no subterms, an empty ul block is generated. And ul blocks are built off li elements. I mean, something like this:

<ul>
  <li class="collapsed"><a href="/..." title="...">...</a></li>
  <li class="expanded"><a href="/..." title="..." class="active">...</a>
    <ul>
    </ul>
  </li>
  <ul>
    <li class="leaf"><a href="/..." title="..." class="active">...</a></li>
  </ul>
  <li class="collapsed"><a href="/..." title="...">...</a></li>
  <li class="collapsed"><a href="/..." title="...">...</a></li>
</ul>

Note the first inner ul block is empty, it causes ugly effects when borders (or margins/padding) are defined around ul elements. The second inner ul block is generated after a /li tag, which is not valid XHTML.

CommentFileSizeAuthor
#6 drupal_screenshot_sample_12.png103.08 KBPatrick Nelson
#4 taxonomy_context2.patch2.36 KBmarkus_petrux
#1 taxonomy_context_0.patch1.72 KBmarkus_petrux

Comments

markus_petrux’s picture

markus_petrux commented
Status: Active » Needs review
StatusFileSize
new taxonomy_context_0.patch1.72 KB

Here's a patch that would fix these issues.

markus_petrux’s picture

markus_petrux commented

You may also want to remove this code from taxonomy_context_show_children():

      if ($c == $context->tid) {
        $params['class'] = 'active';
      }

And this code from taxonomy_context_show_nodes():

    if ($node->nid == $context->nid) {
      $params['class'] = 'active';
    }

Drupal core now, automatically adds the active class to links.

markus_petrux’s picture

markus_petrux commented

Just wanted to mention that I reported the class="active" issue in Drupal core, but they won't fix that.
http://drupal.org/node/45982

...so my previous comment might be necessary here in order to generate cleaner HTML for these blocks.

markus_petrux’s picture

markus_petrux commented
StatusFileSize
new taxonomy_context2.patch2.36 KB

Please, disregard the first patch. The one attached here should fix all issues.

1) Let function l() deal with class="ative".

2) Use ul.menu rather than div.menu, even for inner UL blocks (I forgot this), just like the current menu subsystem works.

HTH+

nedjo’s picture

nedjo
he/him/his
Primary language English
commented

Thanks for these improvements, I'll review and apply them as soon as I have a chance.

Patrick Nelson’s picture

Patrick Nelson commented
StatusFileSize
new drupal_screenshot_sample_12.png103.08 KB

I've applied this patch and there are no problems as far as I can see but markus_petrux, could you post up a screenshot of what a listing under this module should look like. The reason I ask is because I have no idea what it should like and my listings are just a long (lots of terms!) display in rather large text with the node listings for the current term displayed in a similarly long list beneath (see attached screenshot).

This is regardless of whether Use style for term and subterm display: is Enabled or Disabled.

Also, I can see nowhere where the tab.gif and tab_on.gif images included in the module are used?

markus_petrux’s picture

markus_petrux commented

Sorry, I do not have at hand an example with lots of items to post. Though, the patch just tries to adapt to changes made to core in regards to CSS used for ULs (what it was div.menu in 4.6, it is now ul.menu in 4.7, and class="active" is set automatically).

I guess, with this patch, the result would be similar to the same thing you got in 4.6, whereas without this patch, you would have to style div.menu, and you would get something like class="active active".

Also, I can see nowhere where the tab.gif and tab_on.gif images included in the module are used?

Me neither. Probably remainder of older versions?

nedjo’s picture

nedjo
he/him/his
Primary language English
commented

Thanks markus_petrux, please go ahead and apply this patch.

markus_petrux’s picture

markus_petrux commented

Really? Can I touch someone else's module? :-o

I would be glad to do so... sorry, please, confirm, I can't believe it :-)

nedjo’s picture

nedjo
he/him/his
Primary language English
commented

Yes, please go ahead and apply this, both to HEAD and the 4.7 branch, then mark the issue as fixed. Thanks!

markus_petrux’s picture

markus_petrux commented
Status: Needs review » Fixed

Sweet! :-)

Committed to HEAD and to the DRUPAL-4-7 branch.

Anonymous’s picture

(not verified) commented
Status: Fixed » Closed (fixed)