Next in my series of Drupal-core-HTML cleanups comes a patch for feed pages. This first patch for aggregator (yes there will be more, trying to keep things simple and easy to review) cleans up the feed pages specifically. This patch fixes a few things:

1. removes hardcoded DIV that was inaccessible to themers, moving it into appropriate theme function
2. fixes wrong icon being used (XML icon instead of feed icon)
3. Adds new CSS classes for themers
4. Cleans up the default feed view (more aesthetically pleasing)

In the themes I have worked on and on my new upcoming 4.7 theme, styling feeds has always been a pain. Why? The default theme function just *sucks*. Very hard to theme, as a result, I *always* have to override this function so I can add in some classes to be used in my CSS. With this proposed patch, core will spit out better feed pages. As a result, I (and probably many other themers) don't have to override this function and add back in classes so we can theme appropriately. Overall, this makes theming a bit easier.

And yes, I will have some more patches for aggregator that mimic this reasoning ... easier to digest small chunks though ;-)

CommentFileSizeAuthor
#6 more_aggregator_fixes_0.patch5.54 KBUwe Hermann
#5 more_aggregator_fixes.patch2.79 KBUwe Hermann
#3 drupal_41.patch5.21 KBm3avrck
#1 feed_2.png10.48 KBm3avrck
drupal_36.patch5.13 KBm3avrck

Comments

m3avrck’s picture

m3avrck commented
StatusFileSize
new feed_2.png10.48 KB

And here's a before and after shot of how much better it looks (note I had a wide display with these screen shots, but figure how much cleaner it'll look on small widths too).

dries’s picture

dries commented

Please double-check to ensure that all input is properly escaped. I haven't reviewed this patch. I just wanted to point out that some of the changes are potentially prone to XSS attacks.

m3avrck’s picture

m3avrck commented
StatusFileSize
new drupal_41.patch5.21 KB

Dries, yes my patch actually added in some filtering that was not there. But attached is a better patch which also filters the image as well (this was not done anywhere) before it is inserted into the DB. It also cleans up the logic in the theme function, making it quite a bit tidier.

Should be good to go now.

m3avrck’s picture

m3avrck commented

Yeah not only does this cleanup the display, it also cleans up potential vunerabilities, double whammy of a patch ;-)

Uwe Hermann’s picture

Uwe Hermann commented
Status: Needs review » Reviewed & tested by the community
StatusFileSize
new more_aggregator_fixes.patch2.79 KB

Coding style looks ok, patch works and contains good improvements, patch fixes possible security problems. Here' a new patch.

Changes:
1. Applies without warnings.
2. Adds spaces after colons in drupal.css, e.g. "margin-top:0;" -> "margin-top: 0;" for consistency reasons.

Ready to be committed, IMHO.

Uwe Hermann’s picture

Uwe Hermann commented
StatusFileSize
new more_aggregator_fixes_0.patch5.54 KB

Oops, broken patch. This one should work.

m3avrck’s picture

m3avrck commented

I'm ok with those minor tweaks, yet another RTC!

dries’s picture

dries commented
Status: Reviewed & tested by the community » Needs work

I committed the patches but there was a drupal.css clash. Please double-check.

m3avrck’s picture

m3avrck commented
Status: Needs work » Fixed

All of this CSS went in correctly, the trouble CSS was in the 2nd patch, please see this issue for the patch: http://drupal.org/node/45228

Anonymous’s picture

(not verified) commented
Status: Fixed » Closed (fixed)