Hello,

I want to start by saying I really like this module and thank you for creating it. Hopefully my request isn't too much trouble.

Since the site map page does not output a heading1 page title (which is not a problem), I have to add that in the message area provided on the admin page. Putting a heading within a paragraph is not valid. I could also see wanting to put all kinds of other things in that area like multiple paragraphs, lists, images, or divs and none of these should be wrapped in paragraph tags. It would be perfect if it were just wrapped in the div and everything inside was added in the input field in the admin area.

So I would like to suggest removing the paragraph tags from the output of the site map message. I believe it is the first function in the module file. I am not a programmer so I don't know how to create a patch but I assume the code would look something like this.

/**
 * Implementation of hook_help().
 */
function site_map_help($section) {
  switch ($section) {
    case 'sitemap':
      $output = _sitemap_get_message();
      return $output ? $output : '';
  }
}

Thanks

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

hass’s picture

Status: Active » Fixed

Seems no more a case... page have a heading and sitemap message is correctly inside a paragraph - or I do not understand anything.

eff_shaped’s picture

Is this really fixed?

I'm interested in altering the position of site map message - in D6. Trying to access the message output in my theme. Is this possible?

It seems that using hook_help() to add a paragraph to the page makes the output less themeable.
It's not really help text is it? More of a description...
My theme inserts the help text above the breadcrumb; which is not where I want it.

I've used a hard coded text solution in my template.php
I'd much rather use the site map message from the module settings page - more usable for site editors.

This is the code in D6 sitemap.module:

 * Implementation of hook_help().
 */
function site_map_help($path, $arg) {
  switch ($path) {
    case 'sitemap':
      $output = _sitemap_get_message();
      return $output ? '<p>'. $output .'</p>' : '';
  }
}

This is a fine module - I want to keep using it and for it to look good also.

hass’s picture

The hook_help is not for your output. Take a look - I'm not sure if there is a site_map.tpl.php available.

eff_shaped’s picture

A site_map.tpl.php would be awesome. I wish there was one. *sigh*

frjo’s picture

Version: 5.x-1.2 » 6.x-1.x-dev
Status: Fixed » Active

Using hook_help to output "site_map_message" is an old leftover. Now that you brought it up I think it makes more sense to move it to theme_site_map_display().

What do you think?

A template_preprocess_site_map() plus a site_map.tpl.php is an interesting idea. If anyone knows how to do it please submit a patch. Otherwise I will take a look at it sooner or later.

hass’s picture

Title: Site Map Message Output » Add site_map.tpl.php support for custom theming
hass’s picture

Title: Add site_map.tpl.php support for custom theming » Add (node/block)-site_map.tpl.php support for custom theming

Sitemap is more like a node... and someone else asked for a sitemap in a block... http://drupal.org/node/358426

eff_shaped’s picture

Title: Add (node/block)-site_map.tpl.php support for custom theming » Site Map Message Output

@frjo - Putting the message into a themeable place would be a superb development.

Since I'm using the theme_site_map_display() in my theme template.php, it would work well for me.

And it would certainly add to themabilty to have that tpl file.

eff_shaped’s picture

Title: Site Map Message Output » Add (node/block)-site_map.tpl.php support for custom theming

sorry, somehow I changed the title back in error. Correcting that.

beyond67’s picture

Has any progress been made on themeing the sitemap? I would like to output the tags in multiple columns, instead a single long column.

tic2000’s picture

Version: 6.x-1.x-dev » 6.x-2.x-dev
Assigned: Unassigned » tic2000
Status: Active » Needs work
FileSize
6.02 KB

Here is a patch. What it does?

1. On the settings page adds an input format to the text area used for the message.
2. Outputs the message using check_markup().
3. Moves the template for the final output to a file instead of a function.
4. Wraps each of the sections in a div and adds a class to them. Of course those can be changed in the template file.
5. Moves the commentrss in its own theme function.

1 and 2 will help solve the problem OP had.
3-5 adds the site-map.tpl.php file.

Right now only for the page. If this is reviewed and hopefully committed I will create a patch to make the block a template file too if it will still be desired.

The patch is for the 2.x-dev version which should be the one to add new features to.

tic2000’s picture

Status: Needs work » Needs review

wrong status

hass’s picture

Looks promissing... There is only one bug at "additional" where you missed to remove a dot ".="

I can only speak codewise... Untested. I will try to test asap. Really hope to get this cleanup in...

tic2000’s picture

I did not miss. It's in a foreach loop so it has to add to the string each loop.

hass’s picture

Ups, ok... my mobile phone display was a bit small for a review :-)

hass’s picture

hass’s picture

Marked #747032: use theme functions as duplicate

tic2000’s picture

@hass
Please stop doing that.
None of those 2 issues are duplicates. This issue should have been split in 3 smaller one.

1. Don't output the message in paragraph tags.
2. Use a template file for the final output.
3. Use a theme function for commentrss output.

Not any theme issue is in the scope of this one and won't be solved by this one.

This is how issues should be handled and are handled at least for core. One issue at a time in small patches easy to review and commit and easy to roll back in case of a problem.

hass’s picture

It is. You may have not understood theming or what the people are asking for

tic2000’s picture

If they don't ask for a template file for the final output or removing the p tags around the message then it's not a duplicate.
Like I said not all theme issues will be solved by this one.

frjo’s picture

FileSize
10.25 KB

A nice patch tic2000, this I will commit after we resolved some minor issues.

I have attached a revised patch. Moving all theme functions to site_map.theme.inc, rename theme_site_map_commentrss to theme_site_map_rss_legend and other minor stuff.

My main concern is that we know end up with HTML code like this:

<div class="site-map-front-page">
  <div class="sitemap-box sitemap-front">
    <h2 class="title">Front page</h2>
    <div class="content">[…]</div>
  </div>
</div>

That is one div to much I think. One is from site-map.tpl.php and the other from theme_site_map_box().

Any good suggestions?

tic2000’s picture

Status: Needs review » Needs work

I think it's good practice to wrap all the output in a div with a class for easier css theming. Otherwise, for garland for example, everything will be wrapped in a div with class clear-block.
Front page has only one div sitemap-box inside and the wrapper may seem to much, but the menus and taxonomies may have more and that's where a wrapper is useful.

But in the end, whatever is fine. The template file gives the option for everyone to add or remove that div if they so desire. So it's your decision.

Your patch is missing site-map.tpl.php.
If you did create the theme file I think you should move the preprocess function there too.

For the missing file I set to cnw

tic2000’s picture

Status: Needs work » Needs review
FileSize
12.65 KB

Patch to solve the problem and implement the suggestion in #22.

I also made some small changes in phpdoc section of the files to conform to drupal standards. (empty line between @file and the short description).

Again, if you feel like removing those wrapper divs feel free to do so. I don't think it's an issue worth holding this back and it can be easy modified by the user.

frjo’s picture

FileSize
15.36 KB

Reworked the class names a bit, what do you think?

I believe we have something to commit know, thanks for contributing this!

Do you believe I should make a 2.0 release and make it the recommended version?

tic2000’s picture

I see no critical issue open for 6.x-2.x-dev.

Unless someone has any critical issue I think a 2.0 release could be done.

IMO the patch is RTBC, but I did contributed to it and I can't set it as RTBC.

tic2000’s picture

+++ site_map.module	21 May 2010 18:55:27 -0000
@@ -546,7 +412,7 @@ function _site_map_taxonomy_tree($vid, $
+  $output = theme('site_map_box', $title, $output, 'site-map-terms-box sitemap-terms-box-'. $vid . $class);

You should have consistency with the class names. Use "site-map" everywhere.

Powered by Dreditor.

hass’s picture

removed

frjo’s picture

FileSize
17.44 KB

Hopefully the last patch. Fixed consistency for the class names.

tic2000’s picture

Yes. Enough scope creep in this issue.
My comment was only about that piece of code. I didn't even see you have "sitemap" in a lot of other places. It should have been another issue opened for that.

I think this is ready to fly.

frjo’s picture

Status: Needs review » Fixed

Committed to 6-2-dev. Will make a release shortly.

hass’s picture

How about blocks template?

frjo’s picture

To add a template for the current site map block is unnecessary I believe, the block is very simple and small.

A feature request to add a new block that contains the site map itself, and a template for it, should be done in a new issue.

tic2000’s picture

I agree. The current block doesn't require a template file. But it could use a theme function.
Anyway, for that a new issue should be opened.

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.

davidwhthomas’s picture

Thanks for this -dev commit, just what this module needed.
Tested and working fine. Even preserved the existing styling.
Looking forward to the recommended/stable release.
DT

ajlangsdon’s picture

In my site map, i am only posting the Primary & Secondary Links. There are about 100 primary and 20 secondary. Is it possible through theming or even using views to get 60 to display on one side and 60 on the other? It would display the first 60 links for primary in left column, and 61-100 in the right column. The 20 secondary would also be in second column. Any help on this would be excellent. Thanks.