As stated in my previous cleanup here is the next patch in my series of Drupal markup cleanups.

This patch fixes the following:

- cleans up specific feed item pages
- uses admin medium date setting instead of a hardcoded one
- adds some missing HTML filters to prevent possible vunerbilities
- improves generated HTML for better semantics and theming

(note CSS classes follow suit with classes used in previous post, to try and clean up the naming convention and make it consistent)

Before code:

<div class="news-item">
 <div class="date">02:21</div>
 <div class="body">
  <div class="title"><span class="source"><a href="/?q=aggregator/sources/5" >NPR</a>:</span>
<a href="http://www.npr.org/templates/story/story.php?storyId=5160916&amp;amp;ft=1&amp;amp;f=1001">GOP Pushes Lobbying Reform in Scandals&#039; Wake</a></div>

  <div class="description">Republican congressional leaders outline proposed changes in the way Congress interacts with lobbyists, including banning privately funded travel by members of Congress. The proposed changes come in the wake of lobbyist Jack Abramoff's guilty plea and other GOP-related corruption charges.</div>
  <div class="categories">Categories: <a href="/?q=aggregator/categories/1" >News</a></div>
 </div>
</div>

New code:

<div class="feed-item">
<h3 class="feed-item-title"><a href="http://www.npr.org/templates/story/story.php?storyId=5160916&amp;amp;ft=1&amp;amp;f=1001">GOP Pushes Lobbying Reform in Scandals&#039; Wake</a></h3>
<div class="feed-item-meta"><a href="/?q=aggregator/sources/5" class="feed-item-source">NPR</a> - <span class="feed-item-date">1 hour 55 min ago</span></div>
<div class="feed-item-body">Republican congressional leaders outline proposed changes in the way Congress interacts with lobbyists, including banning privately funded travel by members of Congress. The proposed changes come in the wake of lobbyist Jack Abramoff's guilty plea and other GOP-related corruption charges.</div>
<div class="feed-item-categories">Categories: <a href="/?q=aggregator/categories/1" >News</a></div>
</div>

Comments

m3avrck’s picture

StatusFileSize
new27.24 KB

Before and after screens.

m3avrck’s picture

Forgot to mention, this brings up listing multiple sources to be more in line with how Google News works: http://news.google.com/?ned=us&topic=t ... *much* cleaner, easier to theme, etc.

Uwe Hermann’s picture

StatusFileSize
new3.72 KB

Coding style is ok, patch works as expected, patch fixes potential security problems, output looks good. Great work!

Here's a new patch, though, with two minor changes:
1. Patch applies without warnings.
2. Bigger headlines (1.3em), looks better IMHO.

Other than that it's the same.

Uwe Hermann’s picture

Status: Needs review » Reviewed & tested by the community
m3avrck’s picture

Bigger headlines work for me, nice catch! RTC!

m3avrck’s picture

Bigger headlines work for me, nice catch! RTC!

Bèr Kessels’s picture

StatusFileSize
new125.36 KB

here is a before

Bèr Kessels’s picture

StatusFileSize
new118.49 KB

... and an after

Bèr Kessels’s picture

Status: Reviewed & tested by the community » Needs work

Though all in all i hardly see any improvements on the CSS part. The HTML is a lot cleaner, but the CSS is still *very* aggressive.

drupal.css should not:
* set margins
* define lines
* enforce font-sizes
* set paddings.

Bèr Kessels’s picture

Status: Needs work » Reviewed & tested by the community

[23:10:27] berkes: there is no way we can achieve *not* putting that stuff into Drupal.css at this point, i agree 100%
[23:11:04] m3avrck: you mean we should leave this for now, and in future move stuff out of Drupal.css instead?
[23:11:11] berkes: exactly
[23:11:24] no way we'll get drupal.css removed for 4.7 release

This is correct. setting back to 'ready...'

m3avrck’s picture

Ber, while I agree 100%, that is a *much* larger issue in scope and more of a 4.8 issue, dealing with do we include drupal.css or not? It will be there for the 4.7 release, that's not up for debate.

My goal of these patches is to indeed clean up the markup generated by Drupal and add appropriate or fix classes in drupal.css for now. This will make theming *much* easier. Sure, in my upcoming theme I *will* make sure drupal.css is *not* loaded, but the theming system at this stage cannot support *not* having that file.

For 4.8, I think we can ultimately move drupal.css out, but that is, like I said, a much larger and different debate.

dries’s picture

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

Status: Needs work » Reviewed & tested by the community
StatusFileSize
new1.04 KB

Dries, here is the new patch. The clash between the two occured on one line, was a simple fix. With this commited, the CSS for the above patch and this one: http://drupal.org/node/45025 will all be in sync.

dries’s picture

Status: Reviewed & tested by the community » Fixed

Committed to CVS.

Anonymous’s picture

Status: Fixed » Closed (fixed)