Currently, if there is more than one RSS feed per page (easily achievable with the new drupal_add_feed()) it's hard to distinguish between the feed icons that are displayed to the user on the page without inspecting the destination, since they are all have identical alt and title attributes of "Syndicate content".

The attached patch modifies the theme function to accept an optional title parameter for each feed and uses it for the title attribute of the feed icon. The alt attribute remains unchanged, and the title defaults to the old "Syndicate content" if a title is not supplied.

The patch also updates drupal_add_feed() to take advantage of the new title parameter.

CommentFileSizeAuthor
#26 patch_127.txt3.25 KBwebernet
#24 patch_126.txt3.23 KBwebernet
#23 patch_123.txt3.23 KBwebernet
#21 drupal6-feature_feed_icon_usability.patch3.23 KBGurpartap Singh
#18 patch_118.txt2.94 KBwebernet
#17 patch_91.txt2.93 KBwebernet
#8 patch_65.txt1.54 KBwebernet
#6 patch_64.txt1.53 KBwebernet
patch_61.txt1.49 KBwebernet
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
CreditAttribution: webchick commented
Version: 5.x-dev » 6.x-dev

Hm. This is an API change, and we are well into the freeze and even probably looking at RC within a few weeks.

Moving to 6.x-dev. Sorry. :\

As a workaround for 5.x-dev, you could use func_get_args in your theme override to allow additional parameters to be passed in.

webernet’s picture

webernet CreditAttribution: webernet commented

I was hoping that it might be able to get into 5 since it was minor API change (adding an optional parameter) and is backwards compatible, not to mention it dramatically helps the usability of multiple feeds for end users.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
CreditAttribution: webchick commented

You can try. ;) Talk to drumm.

webernet’s picture

webernet CreditAttribution: webernet commented
Version: 6.x-dev » 5.x-dev

I'm moving this back to 5.x-dev in order to get a second opinion on whether it can get in from Drumm/Dries/Steven.

Steven’s picture

Steven CreditAttribution: Steven commented
Status: Needs review » Needs work

We do not apply t() to variables.

Also, I'm generally okay with this patch, but only on the condition that all locations that actually use drupal_add_feed() are verified to have a descriptive title that describes the content.

webernet’s picture

webernet CreditAttribution: webernet commented
Status: Needs work » Needs review
FileSize
patch_64.txt1.53 KB

All of the uses of drupal_add_feed() in core already provide a valid title (generally "RSS - Page title").

Attached patch fixes the t() bug.

RobRoy’s picture

RobRoy CreditAttribution: RobRoy commented
Status: Needs review » Needs work

I'd say have $title = NULL, then do an isset($title) instead just in case people want to put in an empty title for some reason.

webernet’s picture

webernet CreditAttribution: webernet commented
Status: Needs work » Needs review
FileSize
patch_65.txt1.54 KB

I can't think of a good reason for an empty title, but here's the patch using isset().

Steven’s picture

Steven CreditAttribution: Steven commented 
    drupal_add_feed($feed_url, variable_get('site_name', 'Drupal') .' '. t('RSS'));

(node.module)

Doesn't seem very descriptive.

webernet’s picture

webernet CreditAttribution: webernet commented

What would be the preferred RSS title for all of core?

Currently:
- taxonomy, forum, and blog modules all use similar, but not identical implementations of "RSS - $title"
- node module uses "Site name RSS"
- aggregator uses "Site name aggregator"

Also, does "RSS" need to be translatable?

Steven’s picture

Steven CreditAttribution: Steven commented

RSS is a standard and is not translated. However, it seems weird to have 'RSS' in the global content feed, but not in the other feeds.

I don't mind adding RSS to all titles, but the global node/feed feed should have a descriptive word too. Perhaps just 'Front page' ? It's consistent with the wording used elsewhere (e.g. promote to front page).

webernet’s picture

webernet CreditAttribution: webernet commented

I'm not sure what to do about the text. The feed itself likely uses a different title anyways. This title is only for the RSS icons themselves and the alternate link in the HTML source.

I think it's safe to leave them as is for now and open a separate issue to standardize them throughout core if deemed necessary.

Dries’s picture

Dries CreditAttribution: Dries commented 
+function theme_feed_icon($url, $title = NULL) {
+  if (!isset($title)) {
+    $title = t('Syndicate content');
+  }
+  if ($image = theme('image', 'misc/feed.png', t('Syndicate content'), $title)) {

Sorry but I do think that is a little bit weird and I'm not sure I'm a fan of handling defaults that way.

webernet’s picture

webernet CreditAttribution: webernet commented

@Dries: My first patch put the default into the function declaration, but Steven didn't like it since it meant putting the users title through t()...

If anyone has any other suggestions, feel free to comment...

webernet’s picture

webernet CreditAttribution: webernet commented
Title: Improve the theming of feed icons » Improve the usability of feed icons

Changing title to better reflect what the patch does.

Having multiple feed icons on a page that are identical except for the link destination is a usability bug.

drumm’s picture

drumm
he/him
Location NY, US
CreditAttribution: drumm commented
Version: 5.x-dev » 6.x-dev
webernet’s picture

webernet CreditAttribution: webernet commented
Category: task » feature
FileSize
patch_91.txt2.93 KB

New patch:
- No longer uses a default value for the title.
- Updates all core usage of theme_feed_icon to provide a title.

webernet’s picture

webernet CreditAttribution: webernet commented
FileSize
patch_118.txt2.94 KB

Rerolled.

Dries’s picture

Dries CreditAttribution: Dries commented

Looks good to me. Feel free to mark this RTBC after another test/review.

Gurpartap Singh’s picture

Gurpartap Singh CreditAttribution: Gurpartap Singh commented

You forgot to add the new argument to drupal_common_themes(), rest seems great. Think it's RTBC.

Gurpartap Singh’s picture

Gurpartap Singh CreditAttribution: Gurpartap Singh commented
FileSize
drupal6-feature_feed_icon_usability.patch3.23 KB

Here's the patch fixing that.

Sutharsan’s picture

Sutharsan CreditAttribution: Sutharsan commented

I believe that $title should not be compulsory in theme_feed_icon.

function theme_feed_icon($url, $title)

should be

function theme_feed_icon($url, $title = '')

Unless we want to force the specification of a title.

webernet’s picture

webernet CreditAttribution: webernet commented
FileSize
patch_123.txt3.23 KB

We want to require the use of descriptive titles.

Rerolled to remove offsets.

webernet’s picture

webernet CreditAttribution: webernet commented
FileSize
patch_126.txt3.23 KB

Rerolled to remove offsets.

drewish’s picture

drewish CreditAttribution: drewish commented

+1 simple change, the only thing that seems a little odd is:

 function theme_aggregator_feed($feed) {
   $output  = '<div class="feed-source">';
-  $output .= theme('feed_icon', $feed->url) ."\n";
+  $output .= theme('feed_icon', $feed->url, $feed->title .' '. t('feed')) ."\n";

wouldn't it be better to provide the translator a bit more context?:

+  $output .= theme('feed_icon', $feed->url, t('!title feed'. array('!title' => $feed->title))) ."\n";
webernet’s picture

webernet CreditAttribution: webernet commented
FileSize
patch_127.txt3.25 KB

Seems like a reasonable change. Rerolled with:

t('!title feed', array('!title' => $feed->title))
drewish’s picture

drewish CreditAttribution: drewish commented
Status: Needs review » Reviewed & tested by the community

it's a small change so hopefully it's not too presumptuous to RTBC this

Sutharsan’s picture

Sutharsan CreditAttribution: Sutharsan commented

I very much agree with #25 of drewish. In many languages the translation of 'feed' will precede the title.
Rechecked the patch and everything seems Ok to me.

Dries’s picture

Dries CreditAttribution: Dries commented
Status: Reviewed & tested by the community » Fixed

Committed. Small but important usability improvement. Thanks.

Anonymous’s picture

(not verified) CreditAttribution: commented
Status: Fixed » Closed (fixed)
Michsk’s picture

Michsk CreditAttribution: Michsk commented

how do i implement the last patch ??? :P