Closed (fixed)
Project:
Drupal core
Version:
x.y.z
Component:
theme system
Priority:
Minor
Category:
Task
Assigned:
Reporter:
Created:
18 Jan 2006 at 04:36 UTC
Updated:
4 Feb 2006 at 08:32 UTC
Jump to comment: Most recent file
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;ft=1&amp;f=1001">GOP Pushes Lobbying Reform in Scandals' 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;ft=1&amp;f=1001">GOP Pushes Lobbying Reform in Scandals' 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>
| Comment | File | Size | Author |
|---|---|---|---|
| #13 | drupal.css_2.patch | 1.04 KB | m3avrck |
| #8 | after_0.png | 118.49 KB | Bèr Kessels |
| #7 | before.png | 125.36 KB | Bèr Kessels |
| #3 | aggregator_fixes.patch | 3.72 KB | Uwe Hermann |
| #1 | feed_3.png | 27.24 KB | m3avrck |
Comments
Comment #1
m3avrck commentedBefore and after screens.
Comment #2
m3avrck commentedForgot 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.
Comment #3
Uwe Hermann commentedCoding 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.
Comment #4
Uwe Hermann commentedComment #5
m3avrck commentedBigger headlines work for me, nice catch! RTC!
Comment #6
m3avrck commentedBigger headlines work for me, nice catch! RTC!
Comment #7
Bèr Kessels commentedhere is a before
Comment #8
Bèr Kessels commented... and an after
Comment #9
Bèr Kessels commentedThough 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.
Comment #10
Bèr Kessels commented[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...'
Comment #11
m3avrck commentedBer, 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.
Comment #12
dries commentedI committed the patches but there was a drupal.css clash. Please double-check.
Comment #13
m3avrck commentedDries, 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.
Comment #14
dries commentedCommitted to CVS.
Comment #15
(not verified) commented