Posted by ToddThomson on December 12, 2008 at 5:53pm
| Project: | Tagadelic |
| Version: | 6.x-1.x-dev |
| Component: | Code |
| Category: | bug report |
| Priority: | normal |
| Assigned: | Unassigned |
| Status: | closed (fixed) |
| Issue tags: | tagadelic |
Issue Summary
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.
Comments
#1
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
#2
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);
}
#3
I like your concept.
http://drupal.org/node/323 -- creating patches
and then Iĺl review it in more detail.
#4
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
#5
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
#6
Any luck? Would like to see this come in too.
#7
Attached patch against DRUPAL-6--1 (1.40.2.4)
#8
#9
http://drupal.org/node/45111
#10
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
#11
Closing "needs work" that has been open for a long time, without anyone working on it.
#12
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!)
#13
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:
<?phpt(', @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.
#14
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.
#15
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!
#16
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 ;)
#17
Hi Ber,
Can you please give a try for this one and let me know, if any issues?
thank you.
Regards,
Vijayachandran.
#18
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...
#19
Thanks Vijayachandran! Patch @ 17 is working for me.
#20
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
#21
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.
#22
Automatically closed -- issue fixed for 2 weeks with no activity.