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.
Comment | File | Size | Author |
---|---|---|---|
#26 | patch_127.txt | 3.25 KB | webernet |
#24 | patch_126.txt | 3.23 KB | webernet |
#23 | patch_123.txt | 3.23 KB | webernet |
#21 | drupal6-feature_feed_icon_usability.patch | 3.23 KB | Gurpartap Singh |
#18 | patch_118.txt | 2.94 KB | webernet |
Comments
Comment #1
webchickHm. 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.
Comment #2
webernet CreditAttribution: webernet commentedI 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.
Comment #3
webchickYou can try. ;) Talk to drumm.
Comment #4
webernet CreditAttribution: webernet commentedI'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.
Comment #5
Steven CreditAttribution: Steven commentedWe 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.
Comment #6
webernet CreditAttribution: webernet commentedAll of the uses of drupal_add_feed() in core already provide a valid title (generally "RSS - Page title").
Attached patch fixes the t() bug.
Comment #7
RobRoy CreditAttribution: RobRoy commentedI'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.
Comment #8
webernet CreditAttribution: webernet commentedI can't think of a good reason for an empty title, but here's the patch using isset().
Comment #9
Steven CreditAttribution: Steven commented(node.module)
Doesn't seem very descriptive.
Comment #10
webernet CreditAttribution: webernet commentedWhat 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?
Comment #11
Steven CreditAttribution: Steven commentedRSS 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).
Comment #12
webernet CreditAttribution: webernet commentedI'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.
Comment #13
Dries CreditAttribution: Dries commentedSorry but I do think that is a little bit weird and I'm not sure I'm a fan of handling defaults that way.
Comment #14
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...
Comment #15
webernet CreditAttribution: webernet commentedChanging 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.
Comment #16
drummComment #17
webernet CreditAttribution: webernet commentedNew patch:
- No longer uses a default value for the title.
- Updates all core usage of theme_feed_icon to provide a title.
Comment #18
webernet CreditAttribution: webernet commentedRerolled.
Comment #19
Dries CreditAttribution: Dries commentedLooks good to me. Feel free to mark this RTBC after another test/review.
Comment #20
Gurpartap Singh CreditAttribution: Gurpartap Singh commentedYou forgot to add the new argument to
drupal_common_themes()
, rest seems great. Think it's RTBC.Comment #21
Gurpartap Singh CreditAttribution: Gurpartap Singh commentedHere's the patch fixing that.
Comment #22
Sutharsan CreditAttribution: Sutharsan commentedI believe that $title should not be compulsory in theme_feed_icon.
should be
Unless we want to force the specification of a title.
Comment #23
webernet CreditAttribution: webernet commentedWe want to require the use of descriptive titles.
Rerolled to remove offsets.
Comment #24
webernet CreditAttribution: webernet commentedRerolled to remove offsets.
Comment #25
drewish CreditAttribution: drewish commented+1 simple change, the only thing that seems a little odd is:
wouldn't it be better to provide the translator a bit more context?:
Comment #26
webernet CreditAttribution: webernet commentedSeems like a reasonable change. Rerolled with:
Comment #27
drewish CreditAttribution: drewish commentedit's a small change so hopefully it's not too presumptuous to RTBC this
Comment #28
Sutharsan CreditAttribution: Sutharsan commentedI 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.
Comment #29
Dries CreditAttribution: Dries commentedCommitted. Small but important usability improvement. Thanks.
Comment #30
(not verified) CreditAttribution: commentedComment #31
Michsk CreditAttribution: Michsk commentedhow do i implement the last patch ??? :P