I really like your module! However, I have an issue with the following:

Tagadelic blocks provide a title for chunks ( "Tags in Articles", in my case chunk/1 ). There can be a "more tags" link which then brings up all the tags in chunk/1. This works, but the title of the page is "Tags" when it should be "Tags in Articles" ( just as in the block title ). Further, the breadcrumb trail now has "../Tags" with the title underneath also being "Tags". If the breadcrumb trail is followed the page ../tagadelic is reached. The Tagadelic page correctly has the title "Tags", but unfortunately it is a duplicate!

Please let me know if you require any further information.

CommentFileSizeAuthor
#17 346312-patch-4.patch2.39 KBvijaycs85
#14 tagadelic.menu_callback3.patch2.16 KBcaprenter
#12 tagadelic.menu_callback2.patch1.48 KBcaprenter
#7 menu-callback.patch1.14 KBchrissearle

Comments

ToddThomson’s picture

ToddThomson commented

OK, I've read through the code and will make the changes myself. I don't know the protocol for offering a change, but here is my solution:

1) Make the title dynamic through a callback function ( tagadelic_page_title_callback )
2) For each vocabulary referenced in the dynamic URL parameter ( example: 1, 3, 5 ) the title will be rendered as:
"Tags in Articles, Names, Places

Notes:

1) Keep max of 5 in dynamic group - BUG in code after 7 in group will not be fixed ( implode error ).

Tell me how to submit the patch and I'll make it available.

Cheers, Todd

ToddThomson’s picture

ToddThomson commented

You can see the following code at: http://www.achilles-software.com/tagadelic/chunk/1,2

Here is the code for the title callback:

/**
* menu callback to render the tagadelic title
*
* Args:
* $vocs: array of vocabulary IDs
*/
function tagadelic_page_title_callback( $vocs ) {

if ( $vocs == NULL ) {
return 'Tags';
}

foreach ( $vocs as $vid ) {
$vocabulary = taxonomy_vocabulary_load( $vid );
$vocs_list .= t( ', @voc_name', array( '@voc_name' => $vocabulary->name ));
}

// remove leading comma!
return "Tags in" . substr( $vocs_list, 1);
}

Bèr Kessels’s picture

Bèr Kessels commented

I like your concept.

http://drupal.org/node/323 -- creating patches
and then Iĺl review it in more detail.

ToddThomson’s picture

ToddThomson commented

I come from a Windows IDE background and continue to use Visual Studio for most of my work. However, to do PHP/Drupal work I use Eclipse PDT. I have installed CVSNT server for source code control (please let me know if this is not a good choice).

Assumptions:

1) I take it that only module developers and maintainers can open the source code for a module from the official CVS repository.
2) I will need to add the tagadelic module to my local CVS repository.

Cheers, Todd

ToddThomson’s picture

ToddThomson commented

OK.. I found the relevant documentation (drupal and CVS)! ( Drupal docs are SO fragmented )

So for contributing patches:

1) check out the module from cvs.drupal.org. Read only.
2) make changes and test locally
3) move local changes into internal CVS repository.
4) create patch against local file(s) and checked out module from cvs.drupal.org
5) submit patch

I'll give it a shot on Monday!

Cheers, Todd

chrissearle’s picture

chrissearle commented

Any luck? Would like to see this come in too.

chrissearle’s picture

chrissearle commented
StatusFileSize
new menu-callback.patch1.14 KB

Attached patch against DRUPAL-6--1 (1.40.2.4)

chrissearle’s picture

chrissearle commented
Status: Active » Needs review
Bèr Kessels’s picture

Bèr Kessels commented
Priority: Critical » Normal

http://drupal.org/node/45111

Bèr Kessels’s picture

Bèr Kessels commented
Status: Needs review » Needs work

First: please reroll the patch, it fails.

patching file tagadelic.module
Hunk #2 FAILED at 399.
1 out of 2 hunks FAILED -- saving rejects to file tagadelic.module.rej
ber@yasmine:~/Documenten/TAG_tagadelic/unfuddle/tagadelic-git$ 


Second: please adhere to Drupal coding guidelines. Mind the spaces after the brackets.

And maybe you are interested in tracking this against my github version and forking/cloning it there? I see in your patch that you use git. http://github.com/berkes/tagadelic
Bèr Kessels’s picture

Bèr Kessels commented
Status: Needs work » Closed (won't fix)

Closing "needs work" that has been open for a long time, without anyone working on it.

caprenter’s picture

caprenter commented
Status: Closed (won't fix) » Needs review
StatusFileSize
new tagadelic.menu_callback2.patch1.48 KB

Hi

I hope you don't mind me re-opening this, but I need to work on this issue for something I'm doing.

Hopefully I have taken everything done and said in the discussion above and applied it to this patch. I cloned the 6.1.2 version from Github and made the patch against that. I hope that was the right thing to do. It seems to work for my local installation.

Happy to help to make this work, so please get in touch. (I've been using tagadelic since Drupal 4 days, so would be nice to be able to help!)

Bèr Kessels’s picture

Bèr Kessels commented
Status: Needs review » Needs work

Re-opening with an actual patch is fine! Thanks.

Your patch looks right, except for the actual part where the string for the title is built: 

 t(', @voc_name', array('@voc_name' => $vocabulary->name));

just feels not right. It makes translating really weird. Translators should not have to translate strings like ", @voc_name", especially if that comma will be chopped off.

Then, secondly, if someone translates it to, say "or @voc_name", your code (the substr) part will break horrendously.

I already seeked some advice on IRC and am looking for some more insight on how to deal with this dynamic concatenation of a list of items in t(). If you have thoughts, please share them.

caprenter’s picture

caprenter commented
StatusFileSize
new tagadelic.menu_callback3.patch2.16 KB

Thanks for all your help today Ber.

As discussed on IRC, a patch of the latest efforts to do this in a more drupally way.

I'll try to get a finished version done tomorrow.

Bèr Kessels’s picture

Bèr Kessels commented

Needs a bit more work, unfortunately :(

- Should also be applied to the "list" page, not just the "chunks".
- Theme function must be registered in tagadelic_theme().

For the rest, the patch is great! Thanks!

chrissearle’s picture

chrissearle commented

Ref #11 - sorry about that - this one fell off my radar - only spotted it today due to the further updates. No idea why I missed it. Good luck with the latest patch ;)

vijaycs85’s picture

vijaycs85
Location London, UK
commented
Status: Needs work » Patch (to be ported)
Issue tags: +tagadelic
StatusFileSize
new 346312-patch-4.patch2.39 KB

Hi Ber,

Can you please give a try for this one and let me know, if any issues?

thank you.

Regards,
Vijayachandran.

Anonymous’s picture

Anonymous (not verified) commented

Is there any progress on this? It would be fantastic to see this included as it currently can be confusing for users to get to a page that just has the title "tags". It is also not good for SEO - Google Webmaster Tools for instance complains about duplicate titles for pages...

3dloco’s picture

3dloco commented

Thanks Vijayachandran! Patch @ 17 is working for me.

Bèr Kessels’s picture

Bèr Kessels commented
Status: Patch (to be ported) » Fixed

The patch slipped under my radar, because it was not marked "for review".

I like it and have committed it to -dev version. The change is a little too small to issue a new release, but it will be shipped in a new bugfix and minor-changes-release.

Thanks.

Bèr

Anonymous’s picture

Anonymous (not verified) commented

I have just installed the latest dev version and am happy to confirm that it works as advertised. It may only appear to be a small change, but for both usability and SEO it actually helps a lot. Thanks for making this happen.

Status: Fixed » Closed (fixed)
Issue tags: -tagadelic

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

Issue tags: +tagadelic

Restoring issue tags, see #2125755: System messages removed all issue tags during D7 upgrade.