| Project: | Drupal core |
| Version: | 7.x-dev |
| Component: | node system |
| Category: | task |
| Priority: | normal |
| Assigned: | Unassigned |
| Status: | closed (fixed) |
| Issue tags: | dashboard, Favorite-of-Dries, Needs usability review, Novice, UBUserTesting2009, Usability |
Issue Summary
This patch introduces a very simple node block that simply displays the titles of the ten most recently posted nodes. When nodes are created that are neither promoted to the front page nor have a menu item, they virtually disappear from the site, accessible only from admin/content/node or if you happen to memorize the url. By enabling this block by default (in system_install()), we eliminate this problem: as soon as content is posted, it appears in the recent content block, which appears on every page.
This patch is not in any ways complete. It's posted here mainly for feedback. If we were to have a recent content block in core, it would most likely need to have a few more features than what we have here (eg a configurable number of nodes shown, etc).
| Attachment | Size | Status | Test result | Operations |
|---|---|---|---|---|
| recent_content_block.patch | 2.91 KB | Idle | Failed: Failed to apply patch. | View details |
Comments
#1
I like this idea. Off hand, the only issue I can think of is that, during initial site creation, your content list might be skewed toward "about" pages and "contact" pages and other static pages that usually get stuck in a primary or secondary menu (and not highlighted otherwise). Those would roll off, though, as new content was added -- and until they did, the block would be a handy way to get to them.
Plus, you've got a patch, so changing status to CNW based on your comment in #1.
#2
There's also #301902: Allow more users to see the node admin page - which gives you a 'view content' link leading to admin/content/node for users with some content permissions. Seems like this would be a good solution too - and no reason they couldn't both go in.
#3
Reviewable patch now, addressing my comments from the initial issue. Also with a test case now.
#4
Nice ... You could future proof the SELECT query by making it dynamic and adding a node_access tag. Not a big deal though.
#5
The last submitted patch failed testing.
#6
Okay, here is a new patch.
I moved the install bit into node_install (doesn't seem to belong in system since the code is in node). Also changed the block hooks to the new format. Tests pass AFAICT, and the block works :)
#7
The last submitted patch failed testing.
#8
The .install doesn't look quite right - we should be using a defined constant for the block caching mode.
I would also have the entire block content generated by a theme function, rather than building the whole list in hook_block. Also, these sorts of loops I thought we were intended to use
foreach($result...)rather thanwhile(...?#9
I don't know why you would use foreach instead of while, I believe it is the same performance wise and looks about the same. Is there a style guideline on that? I typically use while when using an iterator (like this) and foreach when iterating an actual array, but obviously, it doesn't matter much either way.
In re: the theming, I agree here, but not sure what you propose. The list is already pushed through "theme_item_list". Are you thinking we make a new theme function so that each item can be pushed through it individually and then have the resultant HTML pushed through theme_item_list? Is there an existing theme hook which fits the bill?
Thanks for the review!
Jacob
#10
Mine again for a while. I also think it belongs in system.install for now because that's where all the other default blocks are defined, including for user module, etc. If we want to change that, that belongs in a separate issue.
#11
Correction, it belongs in the various profiles, along with the other queries.
#12
Yes, we moved installing blocks to the default.profile. Also linking to this issue for http://www.drupalusability.org/node/150.
#13
On third thought, yeah, it doesn't belong in expert.profile, that should be for, well, experts. ;)
#14
Patch attached, all tests should be passing now.
#15
The last submitted patch failed testing.
#16
Sorry, trying again...
#17
This patch did what it is supposed to. It looks good to me. I wonder why this hasn't been done sooner.
#18
This needs a screenshot so someone from the usability team can properly review this.
#19
Screenshot attached.
#20
Sorry, removed tag.
#21
-
Does this patch make the block enabled by default? It should be to make it really work for the new user / fresh install
Yes it does from reading the initial issue :)- "Recently posted" doesn't sound quite right to me. Let's use the word "content" here, after all that's what we're referring to.
"Latest content" or "Recent content"?
#22
New patch posted with "Recent content" as the block title. :)
#23
The last submitted patch failed testing.
#24
Uh, what?
I changed a string. That should not prevent installation of Drupal.
I am reposting the same patch.
#25
The last submitted patch failed testing.
#26
Feel like the tests could be cleaned up a lot and might be a tad bit of overkill. I'll review tomorrow when I get back home and on reliable wifi.
#27
Oops, forgot to update the test case for the new title.
#28
Several minor typos, coding style errors, rerolled.
#29
Ok, way cooler patch now. This one actually works, and also adds tests that make sure it actually works. (It was missing a hook implementation in previous versions).
#30
New patch, checks for node status to make sure it's published, includes new tests.
#31
The patch itself looks good, but after looking closely at the problem we are trying to solve "not finding content after created" I believe that it is only a quick fix to a larger problem. As the usability testing of UB2008 and UB2009 seem to have shown was the user not confused by finding his content in admin/content he was soley confused by the fact that once he created a page, it did not initialy show on his homepage or anywhere (if he didn't came across administer yet).
It seems that issue, http://drupal.org/node/395968 takes better care of the "not finding content after created" issue which is a initial experience confusion that is caused by "page" and is not a longer experience kind of issue, which by adding this block we tend to imply.
#32
I guess this is a won't fix, then... :(
#33
With the dashboard looming, this should be something that's available to put there, so re-opening.
#34
The last submitted patch failed testing.
#35
tag
#36
subscribe
#37
An attempt at a straight reroll for the bot's benefit.
#38
Now with patch attached!
#39
The last submitted patch failed testing.
#40
Tests should pass now.
#41
The last submitted patch failed testing.
#42
Hm, test failures I can't reproduce locally. Spiffy. I'll see what I can do when I get a minute to work on this.
In the meantime, I'm wondering if we shouldn't add more content to this block (author, content type, ...) so it better matches the mockup at http://drupal.org/node/544360#comment-1923218 (see the block at the bottom left). I'm not entirely sure what the workflow buttons in that block would do though.
#43
Bumping to critical as per #569190: D7UX: Dashboard contents (this is a must-have for a useful default dashboard)
Overall dashboard issue is @ #544360: D7UX dashboard module
Content-wise, yes, let's do a quick round of sketching ideas for what really should be in here. Not able to read a patch, could you tell me what it displays now?
#44
no really. it's critical :-)
#45
Oh sorry. Screenshot attached.
#46
Since this is intended for the dashboard, we should remove it from the default install profile here, then the node.test hunk could go, and the recent content block test would need to explicitly enable it. With a bit of luck that'll stop the tests from breaking too.
#47
Postponing it for a bit on the overall design discussion for 'recent stuff widgets' on the dashboard. Re-open here once we settle on a design direction in #614410: Patterns for recent-items-blocks on the dashboard. It's a bit jumpy but needs to be considered in a broader context first.
#48
We haz direction (see #614410: Patterns for recent-items-blocks on the dashboard for the design process)
#49
More better image:
#50
Tested this patch - works nice.
I fixed up a call to
theme('feed_icon')(innode_block_view(), which was preventing the patch from applying - after making this change, the patch applied successfully (with offsets).Also fixed the call in the new block to
theme('item_list'), which was affected by the theme() only takes 2 arguments recent API change.This patch fix is courtesy of the Sydney Drupal Users Group code sprint. Yay!!! (Let's not mention how long it took a room full of geeks to fix one line in a patch... oops, I just did...)
Re-rolled patch attached.
#51
The last submitted patch failed testing.
#52
Sorry, last patch was missing file markers. New one attached.
#53
The last submitted patch failed testing.
#54
axyjo requested that failed test be re-tested.
#55
Just want to see which ones still are broken so I can help out with fixing them.
#56
The last submitted patch failed testing.
#57
Currently working on it, but I have to go sleep now. I'll continue tomorrow.
#58
Hm dont , I will post it... Bojhan / webchick asked for it. What I see is that all is well but the test is not adjusted for titles as translateable fields.
#59
Well, almost all titles were, just one assert slipped through and a link pointed to an admin screen that moved meanwhile. Easy fix.
#60
The last submitted patch failed testing.
#61
So what about this?
#62
Here are screenshots:
Recent Content Block:
Recent Content Block settings page:
#63
Ok great, we've got a functional patch again. Now let's compare it to the proposed design:
- Author name is missing, should link to account page (when you have permission to do so
- The 'time ago' info is less important, we can remove it.
- 'Show all' link to the content page is missing
- Header wants to have a linked 'content' , to the same content listing in there.
#64
I think enough of the scaffolding is here that we could give this to a newish contributor to finish up. Not quite a "novice" patch, but should be doable by anyone with a little bit of PHP skills.
#65
I'll be moving this forward. Back in a couple hours with results.
#66
Based on http://drupal.org/files/issues/recent_content_0.patch
Done:
Still to do:
I could really use some clarification on what "Header wants to have a linked 'content' , to the same content listing in there." means.
#67
Test submitted patch...
#68
#69
The last submitted patch failed testing.
#70
CVS settings in Eclipse were set to platform default line endings - this should be fixed
#71
The last submitted patch failed testing.
#72
That vague error usually means that your checkout of Drupal 7 is out of date, and things have moved too fast and it no longer knows how to apply your patch. Could you try updating your source tree (somewhere under the Team menu in Eclipse) and try again?
#73
#74
#75
Do we really need this block to be enabled on Garland by default? I would sleep better without those ugly:
<?php+ // Remove the "recent content" block because it will mess up our assertions
+ // later on if enabled.
+ db_delete('block')
+ ->condition('module', 'node')
+ ->condition('delta', 'recent')
+ ->execute();
?>
#76
I agree.
Pulled all of the hacks from the other hooks and removed the addition to the install profile so that the block will not be shown by default in Garland.
#77
Last patch did not have root directory set correctly.
#78
The last submitted patch failed testing.
#79
Need to modify test cases.
#80
Just a few thoughts. I believe the original premise of the patch was to improve usability, and solve the "disappearing node" problem. This aim is neutralized by not enabling the block on a default installation. I don't know that this is necessarily the right answer - it could be confusing for new users, but evidence suggests that the current system is flawed.
The real solution for the ugly code in the test cases would be to have a separate "test" install profile, which would set up a "clean" Drupal installation. Ideally SimpleTest would allow us to choose the install profile to use, too, on a per-test-case basis. But that belongs in a separate issue.
To limit this issue in scope, I'd like to propose that the usability concerns here be discussed a bit further to clarify whether or not we want the block enabled in the default profile. Any minor code ugliness caused in test cases as a side effect are a fault of the testing framework, not of the usability patch.
#81
cwgordon: Since then, I committed the patch to allow multiple users to see the admin/content page, based on a permission. I believe that addresses the "where did my content go?" problem better than this block. I'd advocate removing it from being visible by default.
#82
Have usability studies (formal or informal) shown that users know to look for their newly created pages at admin/content? I don't think that permissions was the primary issue here. I'm in favor of having the block enabled/visible by default, but I'll defer to webchick's opinion, of course. :)
#83
Unfortunately, we don't have a lot in the way of user testing of Drupal 7's UI yet. But admin/content is *way* more prevalent than admin/content/node used to be, so chances are much higher they will see it.
We really only have the "Where did my content go?" problem for page nodes. I would like to solve that with a toggle in the content type settings to make "Provide a menu item" default to TRUE on certain content types. Not for this issue, though.
#84
webchick, cwgordon: this block is specifically intended as a *dashboard* block, enabled in the default install profile. It's not specifically about fixing 'where's my content' anymore.
#85
Changing title to clarify what we are making this block for.
Edit: and this is the design phase that got us here: #614410: Patterns for recent-items-blocks on the dashboard
#86
Block is missing a lot of functionality.
Todo:
Note: 'Thingnames' support was dropped from specs see webchick's comment
#87
Update
Todo:
Modify default install profile to set block to 'Dashboard main' below 'Management' block in Seven themeCould really use some feedback based on the current table layout
Append 'new' flag to node titles for content new to the userNote: Also applies updated via theme_mark()
Add 'Show all content' link after item nodesNote: 'Thingnames' support was dropped from specs see webchick's comment
Completely rewrite testsNote: Put off for further review as to which tests should be written
Todo after above is complete:
#88
#89
Screen shot of block attached to get feedback on what I should do about the themeing.
#90
Todo:
Could really use some feedback based on the current table layout
http://drupal.org/files/issues/recent-block-screenshot.png
Add edit and delete links for each node based on user privilegesCompletely rewrite testsNote: Put off for further review as to which tests should be written
#91
I really like where this is going. Tagging it as a favorite.
I'd love to see us do a similar block for recent comments too.
#92
I think this would look rather weird in a small width with long titles, either way you do it:
- if you make it retain it's width and space in the table, half of the table will be filled with empty space
- if you don't make it retain it's width and space in the table, and you put the hover on the row (not the cell because you aren't able to hover it then), then the text will wrap and not wrap on mouseenter/mouseleave
+ // TODO: Output is currently UGLY! Need to have "table" attached to heading+ // to match http://img.skitch.com/20091028-mu8je3f7i9f5giyhbyyiaeg35s.png
+ // TODO: Valign top edit and delete columns
to attach the table, you're probably going to have to do some ugly stuff if you want to avoid overriding default padding on the block content area
quick stab:
#dashboard #block-node-recent div.content {padding: 0 0 5px 1px;
}
that makes it look like this on FF & webkit and should hold up on IE, but didn't try
it certainly beats something like
#dashboard #block-node-recent table {margin-top: -10px;
position: absolute;
left: 1px;
width: 99.9%;
}
that's just ugly and doesn't work on small widths
question is, where does it go?
also tested permissions, and they looked good, I just noticed how messed up the dashboard page looks to a user with only that permission, but that's entirely unrelated to this issue :P
#93
oopsie, didn't mean to deprive you of your dreams
#94
#dashboard #block-node-recent div.content {padding: 0 0 5px 1px;
}
seutje, the code above looks great, I tested in IE8. I added the code to dashboard.css at the end of the file - as there are no other *specific* block overrides. The reason I chose to add the code to dashboard.css is that the .content padding is defined within the dashboard.css and not within a theme. Thus it will be up to themes to account for the styling of this block should their global settings not result in an aesthetically pleasing result.
Spurred by the input of seutje, I am debating whether the operation links (edit and delete) should be hidden with JavaScript and shown on hover. I see a lot of potential hangups with this sort of display. The most critical being that I think this would actually reduce usability as you would have to hover the line to see if you can act on the node - kind of like navigation that doesn't list what it is until you hover it (circa 2000 web design).
Todo
Format node items in a table instead of unordered listAbstract functionality in to separate functions for future use, most likely by contributed modules.Make themeable by placing output functionality in to a theme function.Completely rewrite testsNote: Put off for further review as to which tests should be written
Totally unrelated: /me feels special that Dries favorites my work
#95
The last submitted patch failed testing.
#96
Note: for RTL display you might want to reverse
padding: 0 0 5px 1px;topadding: 0 1px 5px 0;but I'm not entirely sure on that+ case 'recent':+ if (user_access('access content')) {
+ $block['subject'] = t('Recent content');
+ $block['content'] = theme('node_recent_block');
+ }
wouldn't it be better to have the block content callback load the data and pass it as a part of the argument passed to theme_node_recent_block? It feels weird right now, as the theming functioning is also responsible for fetching the data...
+ foreach (node_get_recent($number) as $node) {+ $item = array(l($node->title, 'node/' . $node->nid)
+ . theme('mark', array('type' => node_mark($node->nid, $node->changed)))
+ . '<br />' . theme('username', array('account' => user_load($node->uid))));
+ $item[] = node_access('update', $node) ? l(t('edit'), 'node/' . $node->nid . '/edit') : '';
+ $item[] = node_access('delete', $node) ? l(t('delete'), 'node/' . $node->nid . '/delete') : '';
+ $items[] = $item;
+ }
Also, can we drop that
<br />? just wrap the node title and the username in divs if you must, BRs are annoying to style and enforce height, DIVs would be much nicer and easier to override with some padding or margin...#97
yoroy requested that failed test be re-tested.
#98
The last submitted patch failed testing.
#99
CodyCraven requested that failed test be re-tested.
#100
seutje, there isn't a dashboard-rtl.css so I do not know if there is a separate style to apply for right to left support.
I agree on both themeing points and will have them in the next patch. Should we apply any classes to the title and/or username divs? I hate to add any classes when I am not positive about their properness so that I do not make things more difficult for themers down the road.
#101
Does anyone have any input as to whether JavaScript should be used to show operation links (edit and delete) on hover?
#102
I'd advise against it, it makes no sense to me and can only introduce problems
if u do go for a hover thingy, u might have to rethink the whole "using a table without a thead" thing
#103
New patch based on feedback and discussions with seutje and yoroy, also further fleshes out themeing functions.
Todo
Finalize whether JavaScript should be used to hide edit and delete links and show upon hover of corresponding rowOperations shown on row hover will not be added for usability reasons
Completely rewrite testsNote: Put off for further review as to which tests should be written
#104
Portions of code that I need input/feedback on to finish up functionality, read comments in each code block:
1: Node retrieval
/*** Find the most recent nodes that are available to the current user.
*
* @param integer $number
* (optional) The maximum number of nodes to find. Defaults to 10.
* @return
* An array of partial node objects or an empty array if there are no recent
* nodes visible to the current user.
*/
function node_get_recent($number = 10) {
$nodes = db_select('node', 'n')
// Are there any other fields which should be returned for this?
->fields('n', array('uid', 'nid', 'type', 'status', 'title', 'changed'))
// Should we set status as a condition? Isn't the point of the
// module that users do not "lose" content they have created...
->condition('status', NODE_PUBLISHED)
->orderBy('created', 'DESC')
->range(0, $number)
->addTag('node_access')
->execute()
->fetchAll(PDO::FETCH_OBJ);
return $nodes ? $nodes : array();
}
2: Class for 'Show all content' link - we should apply a class name even if it doesn't provide right alignment so that themers can control it without overriding the theme function
if ($rows) {
$output = theme('table', array('rows' => $rows));
// TODO?: Apply a class to this link to match spec
// see: http://drupal.org/node/337947#comment-2294824
$output .= l(t('Show all content'), 'admin/content');
}
3: Classes applied to div tags of themed node names and respective user
// TODO: Add classes to div blocks to allow themers to modify without// overriding theme function, need input on class names to use
$output = '<div>';
$output .= l($node->title, 'node/' . $node->nid);
$output .= theme('mark', array('type' => node_mark($node->nid, $node->changed)));
$output .= '</div><div>';
$output .= theme('username', array('account' => user_load($node->uid)));
$output .= '</div>';
#105
Ok this should be one the final todos.
Todo
Attached is an updated patch. It is functionally identical to the last (http://drupal.org/node/337947#comment-2313748) but I took it upon myself to clear out some of the todos within the patch as I hadn't received any feedback. I also modified the text for the Recent Content Block Configuration page to make it more understandable and simple. A few comments were also removed.
#106
looks pretty decent to me, one little remark:
+ foreach ($variables['nodes'] as $node) {+ $cells = array();
+ $cells[] = theme('node_recent_content', array('node' => $node));
+ $cells[] = node_access('update', $node) ? l(t('edit'), 'node/' . $node->nid . '/edit') : '';
+ $cells[] = node_access('delete', $node) ? l(t('delete'), 'node/' . $node->nid . '/delete') : '';
+ $rows[] = $cells;
+ }
so when the user has no access to edit or delete any nodes, he will have 2 empty columns in the table, or am I tripping again?
#107
Nope you are right, I didn't think that part through. New patch to come.
-- Edit --
After reviewing only showing cells for edit and delete if the user has at least one permission for each one, I have found that a somewhat complex loop would be needed to enforce that only the correct cells exist for the theme_table() call.
As a result there are two options that I see:
#108
I say go for option 1, we did similar with the links on the modules page: #598758: Modules page: add link to its settings page for each module
It allows for consistent layout and alignment even for different permissions
#109
yoroy, thanks for the feedback, we'll go with that option.
I'm also in need of feedback on the three points from http://drupal.org/node/337947#comment-2313988. Once that is received I will only be finishing of up tests and we can commit and close the issue.
#110
With regards to question 1 in #104 I think it would be better to not filter by status. As a site administrator a draft that I create but have not published yet should show up in my recent content block allowing me to quickly find the nodes that I have been working on recently regardless of wether or not they are published.
I suppose making sure that it is apparent in the UI which nodes are not-published would be a good idea.
#111
+++ modules/dashboard/dashboard.css 28 Nov 2009 03:36:30 -0000@@ -113,3 +113,7 @@
\ No newline at end of file
This should not happen. Please always add a newline.
+++ modules/node/node.module 28 Nov 2009 03:36:30 -0000@@ -1994,6 +2000,9 @@
+
+ $blocks['recent']['info'] = t('Recent content');
+
(and elsewhere) Trailing white-space here (blank lines should be blank).
+++ modules/node/node.module 28 Nov 2009 03:36:30 -0000@@ -2001,13 +2010,130 @@
+ * Implementation of hook_block_configure().
(and elsewhere) The new standard for hook summaries is:
Implements hook_foo().
+++ modules/node/node.module 28 Nov 2009 03:36:30 -0000@@ -2001,13 +2010,130 @@
+function node_block_configure($delta = '') {
+ $form = array();
+ if ($delta == 'recent') {
Strange indentation here.
+++ modules/node/node.module 28 Nov 2009 03:36:30 -0000@@ -2001,13 +2010,130 @@
+ '#title' => t('Number of recent content nodes'),
We don't use the term "node" in user-facing text.
+++ modules/node/node.module 28 Nov 2009 03:36:30 -0000@@ -2001,13 +2010,130 @@
+ * @param integer $number
+ * (optional) The maximum number of nodes to find. Defaults to 10.
+ * @return
(perhaps also elsewhere) There should be a blank PHPDoc line between @param and @return.
+++ modules/node/node.module 28 Nov 2009 03:36:30 -0000@@ -2001,13 +2010,130 @@
+ $nodes = db_select('node', 'n')
+ // Are there any other fields which should be returned for this?
+ ->fields('n', array('uid', 'nid', 'type', 'status', 'title', 'changed'))
...
+ ->execute()
+ ->fetchAll(PDO::FETCH_OBJ);
You only should define 'nid' here and do ->fetchCol() after execute() instead.
Afterwards, you do
$nodes = node_load_multiple($nids);
+++ modules/node/node.module 28 Nov 2009 03:36:30 -0000@@ -2001,13 +2010,130 @@
+ // Should we set status as a condition? Isn't the point of the
+ // module that users do not "lose" content they have created...
+ ->condition('status', NODE_PUBLISHED)
We need to dynamically apply this condition depending on whether the current user has the "by-pass node access" permission.
+++ modules/node/node.module 28 Nov 2009 03:36:30 -0000@@ -2001,13 +2010,130 @@
+ ->orderBy('created', 'DESC')
I believe this condition should be 'changed', not 'created'.
+++ modules/node/node.module 28 Nov 2009 03:36:30 -0000@@ -2001,13 +2010,130 @@
+ $cells = array();
+ $cells[] = theme('node_recent_content', array('node' => $node));
+ $cells[] = node_access('update', $node) ? l(t('edit'), 'node/' . $node->nid . '/edit') : '';
+ $cells[] = node_access('delete', $node) ? l(t('delete'), 'node/' . $node->nid . '/delete') : '';
+ $rows[] = $cells;
The common standard in core is:
$row = array();
$row[] = ...;
$row[] = ...;
$rows[] = $row;
+++ modules/node/node.module 28 Nov 2009 03:36:30 -0000@@ -2001,13 +2010,130 @@
+ // TODO?: Apply a class to this link to match spec
+ // see: http://drupal.org/node/337947#comment-2294824
+ $output .= l(t('Show all content'), 'admin/content');
Off-hand, I don't know of another link in core that is similar to this.
The most comparable link class is probably ".read-more", but that's entirely different.
I'd suggest to use
list-all.+++ modules/node/node.module 28 Nov 2009 03:36:30 -0000@@ -2001,13 +2010,130 @@
+ else {
+ $output = t('No content available.');
+ }
This should not happen. Instead, node_block_view() should not return anything, so no block is displayed if there is no content.
+++ modules/node/node.module 28 Nov 2009 03:36:30 -0000@@ -2001,13 +2010,130 @@
+ // TODO: Add classes to div blocks to allow themers to modify without
+ // overriding theme function, need input on class names to use
+ $output = '<div>';
.node-title
.node-author
This review is powered by Dreditor.
#112
Updated patch with all of sun's inputs corrected.
I believe all that is left are tests. Please review and let me know if anything is missing or needs correction.
#113
+++ modules/dashboard/dashboard.css 30 Nov 2009 06:06:45 -0000@@ -113,3 +113,7 @@
+#dashboard #block-node-recent div.content {
+ padding: 0 0 5px 1px;
+}
That is some strange padding. Are you sure this is required? If it really is, then I'd say that we have a styling problem elsewhere in the dashboard.
+++ modules/node/node.module 30 Nov 2009 06:06:46 -0000@@ -1016,30 +1022,30 @@
-
+
ok, hehe... I only wanted _you_ to not introduce new trailing white-space, not really to fix the entire file. ;)
That is, because it's more likely now that your patch will fail to be applied when another patch changing another part of node.module will get in.
I hope we are lucky and this won't happen. If it will, then you'll need to re-roll. ;)
+++ modules/node/node.module 30 Nov 2009 06:06:46 -0000@@ -2006,13 +2015,140 @@
+ case 'recent':
+ if (user_access('access content')) {
+ $block['subject'] = t('Recent content');
+ $block['content'] = theme('node_recent_block', array(
+ 'nodes' => node_get_recent(variable_get('node_recent_block_count', 10)),
+ ));
+ }
+ break;
This does not skip the block output if there are no nodes to display.
if (user_access('access content') && ($nodes = node_get_recent(variable_get('node_recent_block_count', 10)))) {$block['subject'] = t('Recent content');
$block['content'] = theme('node_recent_block', array(
'nodes' => $nodes,
));
}
+++ modules/node/node.module 30 Nov 2009 06:06:46 -0000@@ -2006,13 +2015,140 @@
+ * @param integer $number
We do not specify data types in PHPDoc.
+++ modules/node/node.module 30 Nov 2009 06:06:46 -0000@@ -2006,13 +2015,140 @@
+ if (!user_access('bypass node access')) {
+ // If the user is able to view their own unpublished nodes, allow them
+ // to see these in addition to published nodes. Check that they actually
+ // have some unpublished nodes to view before adding the condition.
+ if (user_access('view own unpublished content') && $own_unpublished = db_query('SELECT nid FROM {node} WHERE uid = :uid AND status = :status', array(':uid' => $GLOBALS['user']->uid, ':status' => NODE_NOT_PUBLISHED))->fetchCol()) {
+ $query->condition(db_or()
+ ->condition('n.status', NODE_PUBLISHED)
+ ->condition('n.nid', $own_unpublished, 'IN')
+ );
+ }
+ else {
+ // If not, restrict the query to published nodes.
+ $query->condition('n.status', NODE_PUBLISHED);
+ }
+ }
oh nice! Copied from node_admin_content(), I guess :)
This really cries for a helper function...
+++ modules/node/node.module 30 Nov 2009 06:06:46 -0000@@ -2006,13 +2015,140 @@
+ $rows = array();
...
+ if ($rows) {
+ $output = theme('table', array('rows' => $rows));
...
+ return $output;
$output is not initialized.
+++ modules/node/node.module 30 Nov 2009 06:06:46 -0000@@ -2006,13 +2015,140 @@
+ $l_options = array('query' => array('destination' => $_GET['q']));
Instead of defining the value for 'query' yourself, you use http://api.drupal.org/api/function/drupal_get_destination/7
This review is powered by Dreditor.
#114
and back to needs work… :)
#115
"That is some strange padding. Are you sure this is required? If it really is, then I'd say that we have a styling problem elsewhere in the dashboard."
I agree. The reason we needed to do this was because the table is supposed to touch the heading. The dashboard.css file defines padding on "#dashboard div.block div.content" that we have to unset.
---
"ok, hehe... I only wanted _you_ to not introduce new trailing white-space, not really to fix the entire file. ;)"
Dries latest commit introduced this white space, I believe. Before I had forgot configure removal of trailing white space in my Eclipse editor on save (recently reinstalled). So if I need to remove this portion I will have to manually remove it. I do update my files before creating patches so that should limit the chance of the patch not being applicable.
---
"This does not skip the block output if there are no nodes to display."
I made all of the changes then had an issue with my merge from the updated CVS file and forgot to add that specific logic back in when I rewrote :\ - corrected in patch
---
"We do not specify data types in PHPDoc."
We do define datatype in phpDoc, it's Drupal's Doxygen that doesn't, I'm having hard time transitioning... - corrected in patch
--
"oh nice! Copied from node_admin_content(), I guess :)"
I totally agree that this needs a helper, and yes - no need to reinvent? :D
---
"$output is not initialized."
Corrected in patch
---
"Instead of defining the value for 'query' yourself..."
Corrected in patch
#116
Forgot to set status to 'needs review'
#117
So, in actually testing this patch, I can't get any content show up in the block.
- Added it to the dashboard
- Ran cron to be sure…
- Created some articles
- The new content is not listed in this block
- Ran cron again…
- No content shown.
Logged in as user/1 on an otherwise unpatched checkout of head. Can somebody duplicate?
#118
@sun
o wow, as I was typing the reason I introduced this I had an epiphany, reset.css and defaults.css both set border-collapse:collapse; but if I disable both of those and have FF fall back to the user-agent default, which is border-collapse:separate;, the issue disappears and the nasty 1px padding is no longer necessary
test and source
so is this an issue in the reset.css and defaults.css and should it be fixed there, or should we override it for this? I'd say the former, but I'm no expert on resets
#119
Should be fixed over there. There's enough weirdness in Seven's CSS that needs clean up, lets not introduce fixes for that somewhere else (like in here :-)
#120
Actually, I can't see why this patch shouldn't work.... didn't test though.
Speaking of tests.... no tests? yoroy wouldn't even have to test something if this patch would ship with its own tests.
+++ modules/dashboard/dashboard.css 30 Nov 2009 21:30:51 -0000@@ -113,3 +113,7 @@
+#dashboard #block-node-recent div.content {
+ padding: 0 0 5px 1px;
+}
So we want to remove this.
+++ modules/node/node.module 30 Nov 2009 21:30:51 -0000@@ -2006,13 +2015,141 @@
+ $block['content'] = theme('node_recent_block', array(
+ 'nodes' => $nodes,
+ ));
No need to write this on multiple lines now.
This review is powered by Dreditor.
#121
Due to debate of whether the fix to dashboard.css should be included, especially considering UI freeze 12/1, attached are two patches and two screenshots. Also the whitespace fixes from HEAD in node.module (seen in last patch) have been removed.
#122
@sun
actually, if we want the table to touch the header, we might still want to override the left, right and top padding of the content area as this is the default that dashboard uses:
#dashboard div.block div.content {padding:10px 5px 5px;
}
which seems reasonable, and if we drop the left, right and top padding defaults, every block on the dashboard will stick to the header, which I don't think is something we want to default to
#123
So I take it this is dead now that UI freeze is done?
#124
No, this is critical for a viable dashboard implementation. We have some extra room for selected issues, this will be one of them. Do you need people testing it before you continue?
#125
+++ modules/node/node.module 1 Dec 2009 04:44:41 -0000@@ -2006,13 +2015,141 @@
+ '#options' => drupal_map_assoc(array(2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16, 17, 18, 19, 20, 25, 30)),
+ );
Do we have exactly these numbers elsewhere? Recent comments block maybe? Could be a helper perhaps but not important.
+++ modules/node/node.module 1 Dec 2009 04:44:41 -0000@@ -2006,13 +2015,141 @@
+/**
+ * Implementation of hook_block_save().
+ */
"Implements".
+++ modules/node/node.module 1 Dec 2009 04:44:41 -0000@@ -2006,13 +2015,141 @@
+/**
+ * Returns a formatted list of recent nodes to be displayed in the recent content block.
+ *
Exceeds 80 chars. I'd just drop the last 8 words (end with 'nodes').
Screenshot looks lovely. Agreed with sun that this could use some tests - ought to be as little as creating some content then visiting the dashboard.
This review is powered by Dreditor.
#126
Yes, these numbers are from comment.module.
I'll fix the comments.
Also, I absolutely want to write the tests for this. I have a short contract in progress right now so I will need some time to get the tests written.
#127
I'll try to start to write tests for this.
#128
This patch uses the 'without_dashboard_css' as its base as I think that it's a separate issue altogether. I also think that the helper for the common numbers is a separate issue, so I didn't work on that in this patch. The test is *largely* based off the recent comments test, so it probably will fail with this patch.
#129
The last submitted patch failed testing.
#130
Here's something that should fix it. Also takes care of the extraneous whitespace.
#131
The last submitted patch failed testing.
#132
Okay, 3rd time lucky. This time, the patch passes on my local machine (all but one of the "Node (not) found in block" messages fail). Looks like it's just a matter of fixing the assert statements. I think I've taken care of all the syntax/typo errors. Also takes into account the change made by #635094: Unify "language neutral" language codes. I need some sleep now, so anybody's welcome to continue on this.
#133
The last submitted patch failed testing.
#134
<?php// Test that a user without the 'access content' permission can not see the block.
$this->drupalLogout();
$this->drupalGet('');
$this->assertNoText($block['title'], t('Block was not found.'));
?>
I think this test fails because the anonymous user has 'access content' permission. You need to create a user without that permission. Also, the comment in that block needs to wrap at 80 characters.
// Test the only the 2 latest nodes are shown and in the proper order.Should be "Test that ...".
#135
Still shows up as 7 fails on my local machine, but let's see what the test bot has to say. Also, fixed both of the comments.
#136
The last submitted patch failed testing.
#137
Let's see how this does now that I figured out how to revoke the 'access content' privilege from anonymous users.
#138
The last submitted patch failed testing.
#139
In your patch, this
$node1->title['zxx'][0]['value']should be this$node1->title[LANGUAGE_NONE][0]['value']in a number of places.Patch contains a number of debug statements.
+ debug('Node "'.$node1->title['zxx'][0]['value'].' created.');// Test that only the 2 latest nodes are shown and in the proper order.$this->assertNoText($node1->title['zxx'][0]['value'], t('Node not found in block.'));
$this->assertText($node2->title['zxx'][0]['value'], t('Node found in block.'));
$this->assertText($node3->title['zxx'][0]['value'], t('Node found in block.'));
$this->assertTrue(strpos($this->drupalGetContent(), $node3->title['zxx'][0]['value']) < strpos($this->drupalGetContent(), $node2->title['zxx'][0]['value']), t('Nodes were ordered correctly in block.'));
I believe the problem with this set of tests is that all of your nodes are getting created with the exact same $node->changed timestamp. You might be able to do something like
<?php$default_settings['changed'] => time() - 100;
?>
#140
Yeah it seemed that the failing tests were due to the fact that the "changed" time of the nodes are all the same so ordering could not be tested properly.
I added the following for node2 and node3. I was attempting to set it in the $default_settings variable but it seems that "drupalCreateNode" overrides the changed value. :(
db_query('UPDATE {node} SET changed = :changed WHERE nid = :nid', array(':changed' => $node1->changed + 100, ':nid' => $node2->nid));db_query('UPDATE {node} SET changed = :changed WHERE nid = :nid', array(':changed' => $node1->changed + 200, ':nid' => $node3->nid));
Also the last set of tests were testing on "admin/structure/block/manage/node/recent/configure" instead of a page where the block would be displayed.
These tests now pass although I do not know what the proper solutions for the changed date would be I would think that we should be able to override the changed value from the passed in $default_settings.
** Made suggested changes from #139 http://drupal.org/node/337947#comment-2341644
Thanks,
Jonathan
#141
#142
Just a quick question. Would using db_update() be better in this case since it's really a pretty simple thing or is db_query more efficient?
#143
db_query() is only supposed to be used for select queries. Any time you're updating or deleting, you have to use the query builder. There's pretty good documentation, fortunately.
#144
Changing "changed" time for node to db_update instead of db_query.
+ db_update('node')+ ->fields(array(
+ 'changed' => $node1->changed + 100,
+ ))
+ ->condition('nid', $node2->nid, '=')
+ ->execute();
...
+ db_update('node')
+ ->fields(array(
+ 'changed' => $node1->changed + 200,
+ ))
+ ->condition('nid', $node3->nid, '=')
+ ->execute();
Thanks,
Jonathan
#145
Looks good to me, but I'll won't mark this as RTBC it since I'm a collaborator of this patch.
#146
I glanced quickly over the code and it looks good. I tried it out and it works fine. I am wondering about the styling... maybe it's a Seven thing, but it seems odd to have a border around the bottom and sides but not the top. Here's a screenshot of it as it appears on the dashboard:
#147
+++ modules/node/node.test 8 Dec 2009 14:10:35 -0000@@ -1170,3 +1170,107 @@ class NodeFeedTestCase extends DrupalWeb
+ protected $admin_user;
+ protected $web_user;
+ protected $anon_user;
+
These are technically not required.
Also note the trailing white-space here.
+++ modules/node/node.test 8 Dec 2009 14:10:35 -0000@@ -1170,3 +1170,107 @@ class NodeFeedTestCase extends DrupalWeb
+ ->condition('nid', $node2->nid, '=')
...
+ ->condition('nid', $node3->nid, '=')
'=' can be removed here.
+++ modules/node/node.test 8 Dec 2009 14:10:35 -0000@@ -1170,3 +1170,107 @@ class NodeFeedTestCase extends DrupalWeb
+ $node3 = $this->drupalCreateNode($default_settings);
+ // Change the changed time for node so that we can test ordering.
(and elsewhere) We could squeeze some blank lines in here to increase readability in the test flow.
+++ modules/node/node.test 8 Dec 2009 14:10:35 -0000@@ -1170,3 +1170,107 @@ class NodeFeedTestCase extends DrupalWeb
+ //$this->assertNoText($block['title'], t('Block was not found.'));
?
+++ modules/node/node.test 8 Dec 2009 14:10:35 -0000@@ -1170,3 +1170,107 @@ class NodeFeedTestCase extends DrupalWeb
+ $this->assertTrue(strpos(strip_tags($this->drupalGetContent()), $node3->title[LANGUAGE_NONE][0]['value']) < strpos(strip_tags($this->drupalGetContent()), $node2->title[LANGUAGE_NONE][0]['value']), t('Nodes were ordered correctly in block.'));
You want to use xpath here instead.
I'm on crack. Are you, too?
#148
@axyjo and @grndlvl
+++ modules/node/node.test 8 Dec 2009 14:10:35 -0000@@ -1170,3 +1170,107 @@ class NodeFeedTestCase extends DrupalWeb
+ //$this->assertNoText($block['title'], t('Block was not found.'));
Looks like we have one test that wasn't finished up. To do this we will need to make a role that does not have the 'access content' permission. This will require changing the permissions of the Anonymous User role. As part of this the $anon_user property can be entirely removed... or we can completely remove this test as I'm not sure it is needed.
// Test that block is not visible without nodes$this->drupalGet('');
$this->assertNoText($block['title'], t('Block was not found.'));
Additionally we can add the above code to the test prior to nodes being added within testRecentNodeBlock().
Thank you two for picking up with these tests. I am under a contract that is finishing up in the next few days and this patch would not be at this stage yet without your help.
@ksenzee The styling issue of the block is due to padding being added to content within dashboard.css. See http://drupal.org/comment/reply/337947#comment-2324118 and previous comments regarding this issue.
This review is powered by Dreditor.
#149
I'll try to reroll it with the suggestions after school today (unless grndlvl beats me to it). Thank you, ksenzee, sun and CodyCraven.
#150
I have applied the suggestions from #147 && #148.
- removed unnecessary protected variable definitions.
- cleaned up
- removed equal signs from condition method.
- replacing check for order of nodes in block to xpath
xpath check
<?php$this->assertTrue($this->xpath('//div[@id="block-node-recent"]/div/table/tbody/tr[position() = 1]/td/div/a[text() = "' . $node3->title[LANGUAGE_NONE][0]['value'] . '"]'), t('Nodes were ordered correctly in block.'));
?>
#151
not getting any node titles
+function theme_node_recent_content($variables) {+ $node = $variables['node'];
+
+ $output = '<div class="node-title">';
+ $output .= l($node->title[LANGUAGE_NONE][0]['safe'], 'node/' . $node->nid);
+ $output .= theme('mark', array('type' => node_mark($node->nid, $node->changed)));
+ $output .= '</div><div class="node-author">';
+ $output .= theme('username', array('account' => user_load($node->uid)));
+ $output .= '</div>';
+
+ return $output;
+}
$node->title[LANGUAGE_NONE][0]['safe']isn't defined, changed this to$node->title[LANGUAGE_NONE][0]['safe_value']also, I noticed that all dashboard blocks got a border by #633086: Dashboard visual design improvements
does this mean we're dropping the border on the table, or should we just add a proper thead with like "title" "author" "operations" or w/e?
coz right now it looks pretty funky with the double border that doesn't span the top of the table
attached patch just changes
$node->title[LANGUAGE_NONE][0]['safe']to$node->title[LANGUAGE_NONE][0]['safe_value']#152
Definately getting node titles now. Liking it. Any chance we can link the word 'content' in the 'Recent content' h2?
As for the styling: why have the borders show at all?
Attached patch should make it look like the above. Bluntly added these styles to dashboard.css because this is a rather dashboard specific override. Happy to be told otherwise ;)
#153
yea see, much better without the borders
but uhm, why did u move the "show all content" to the right?
I think that would require some serious fiddling and it would effectively put the same link on this page 3 times, lil over the top if u ask me :P
I'm also not entirely sure on where to put these styles, as node.module is required and dashboard.module isn't, it would seem a bit silly to be loading these styles all the time, so I guess dashboard.css does seem like the better place to put it as those styles are only loaded on the dashboard
granted, with aggregation, it rly wouldn't matter
#154
Well, both Mark's as my design have links in the header and bottom right in the design:
http://drupal.org/node/337947#comment-2208774
http://www.flickr.com/photos/mboulton/3748580107/sizes/o/
For the header I suspected it would be tricky, so we'll let it slide for now :)
For the right aligned one underneath, the idea was to make the link relate to the operation links.
#155
oh ok, makes sense to me :)
come to think of it, linking a part of the title wouldn't be that hard, just change this to include a link:
+ $block['subject'] = t('Recent content');#156
I was going to test this patch, but it no longer applies for me. Needs a reroll?
#157
yoroy requested that failed test be re-tested.
#158
yoroy requested that failed test be re-tested.
#159
Dries, I just patched up my D7 to match HEAD and yoroy's patch applied fine. Is there some sort of discrepancy between the test server(s) and HEAD?
#160
Here is what I get:
<?phpdeimos:drupal-cvs dries$ patch -p0 < f.p
patching file modules/node/node.module
Hunk #2 succeeded at 2034 (offset 3 lines).
Hunk #3 succeeded at 2044 (offset 3 lines).
patching file modules/node/node.test
patching file profiles/default/default.install
patching file modules/dashboard/dashboard.css
patch unexpectedly ends in middle of line
patch: **** malformed patch at line 335
?>
#161
yeh I'm getting the same
reroll attempt
#162
Applied patch, but none of my content is displaying. :/ See screenshot.
Assuming it's not me causing the above problem, I think standard D7 phrasing for empty text is "No content available."
FYI:
patching file modules/dashboard/dashboard.cssHunk #1 succeeded at 113 (offset -33 lines).
patching file modules/node/node.module
Hunk #1 succeeded at 157 (offset -13 lines).
Hunk #2 succeeded at 1978 (offset -56 lines).
Hunk #3 succeeded at 1988 with fuzz 1 (offset -56 lines).
patching file modules/node/node.test
Hunk #1 succeeded at 1072 (offset -98 lines).
patching file profiles/default/default.install
Hunk #1 succeeded at 121 (offset -75 lines).
#163
This is one of those patches you want to apply to a fresh checkout of head *before* installing, only then can the system know it's there.
#164
- Patch in #161 applies, thanks seutje.
- It works as expected. I added some content and it looked like this:
#165
I reviewed the patch and the formatting looks good and the code (logically) makes sense. So I guess we will just wait on the testing system to stop postponing and hopefully it will still apply if there are any commits prior to this patch being tested.
#166
The last submitted patch failed testing.
#167
bash-3.2$ patch -p0 < ../recentcontent-reroll.patchpatching file modules/dashboard/dashboard.css
patching file modules/node/node.module
patching file modules/node/node.test
patching file profiles/default/default.install
Hunk #1 succeeded at 186 (offset -10 lines).
... o.O
reroll to get rid of offset I suppose
#168
The last submitted patch, , failed testing.
#169
Re-test of from comment #2394650 was requested by @user.
#170
The last submitted patch, , failed testing.
#171
I broketeth it? :(
I don't rly see how I could of broken filefield uploads though... help?
#172
I think this might be to an issues that got committed and not an issue w/ this patch because all the latest patches have failed on w/ the same test failing on testing.drupal.org.
#173
definitely a bad commit
#174
Re-test of 337947-recentcontent-167.patch from comment #167 was requested by CodyCraven.
#175
RTOTHEBC
#176
Applying #167 adds the recent content block as described, but content only appears after the caches have been cleared.
Steps to reproduce:
1. apply patch #167
2. add 'recent content' block to dashboard
3. create some content
3. visit dashboard
expected: see recently created items in 'recent content' block
actual: 'recent content' block remains empty
workaround: clear caches
In contrast: the 'recent comments' block added to the dashboard *does* immediately show recent comments without needing to clear the caches.
#177
The admin dashboard should not be cached?
Like:
/admin/content/node
/admin/content/comment/new
#178
errr, these are blocks and thus act as such
and any block to any module's hook_block and it won't appear until the cache is cleared afaik
as stated by yoroy in #163, one should apply patches prior to installing, pretending it were in the package as downloaded
#179
Re-test of 337947-recentcontent-167.patch from comment #167 was requested by webchick.
#180
The last submitted patch, 337947-recentcontent-167.patch, failed testing.
#181
Sorry, folks but this will need some re-roll action after #571654: Revert node titles as fields.
This is also the next "biggie" patch on my radar to get in before alpha. Could someone re-roll?
#182
I think it's because we haven't looked at color usage here yet :)
#183
rerolled
#184
peximo, your last patch was perfect thank you.
Due to the display of "(empty)" in Dashboard when no content has been created in the system yet, a UX decision has been made by yoroy to output the text "No content available."
The output of this message creates consistency with admin/content which outputs the same message when no content has yet been created.
Attached is the modified patch including a new test case to replace the block not found assertion.
#185
I know you're never supposed to tell a designer that there's too much whitespace, but I think there's too much whitespace. ;) Or at least those gaps on the sides of the table look weird.
As loathe as I am to introduce more obvious visual bugs though, it's also an obvious visual bug for the Dashboard to look so... empty... by default. I also have a feeling these issues may be caused by/fixed in #563390: Seven theme lacks formatting on common html elements such as lists, paragraphs & <code>, and this poor patch has been through quite the hassle on its way towards being committed.
I looked through the code one more time and didn't spot anything obviously horrible, and it comes with tests to prove it works, so w00t!
Committed to HEAD. Marking back to code needs work to sort out the spacing bugs.
#186
#190
Thinking about this while away from computer (currently on BlackBerry)...
Since Garland and Seven are both affected by the weird whitespace in Dashboard for this block I am thinking that the issue is within dashboard.css and not either theme. If this is the case then it will be easy to add a little code to dashboard.css setting the H2 and .content elements to remove the padding that was recently added.
I will be at my computer in a couple hours and check on this if no one beats me to it.
#192
dashboard.css is the culprit.
Attached is a patch which sets the padding on .content for the recent content block to 0.
#193
#194
The last submitted patch, recentcontent-whitespace.patch, failed testing.
#195
Oops, had a path issue within the patch
#196
Bump, just need a quick review by UX team and a yay or nay on the patch so we can close this out.
#197
No screenshot?
#198
Sorry, screen shot attached.
When the issue first arose I believe there was more whitespace, but I may be mistaken.
#199
The "list-all" class introduced by this patch should be "more-link" to be consistent with other blocks that Drupal provides. Please consider changing this.
Here's a screenshot that demonstrates 3 other core blocks that use .more-link:
#200
Go, Jacine, go! :)
#201
No idea what you are RTBC'n, there are like 4 diffrent patches in this issue. It seems Jacine only touches a small part of that,
#202
Here's an updated patch that also removes the CSS the dashboard is providing for this. There is styling for .more-link in system.css that takes care of aligning the text right, and "magically" fixes the whitespace issue. Anything else that is needed to achieve Seven-specific styling should be done in the Seven theme.
Here's a screenshot showing the side-by-side comparison of before and after my patch: http://www.flickr.com/photos/jacine-rodriguez/4354240042/sizes/l/
#203
Sorry, I have a bad habit of deleting the last line at the end of files. Use this patch instead.
#204
There's another problem with this block.
When a user does not have access to edit or delete the nodes in the list, the result is empty table cells. This isn't immediately apparent because neither Seven nor Garland have vertical borders on their tables, but it's not acceptable.
I've updated the patch to fix this too.
EDIT: ignore this patch, see the one in the next comment.
#205
Jeez. Sorry for the inbox spam/bad patch. Please ignore the last one and use this.
#206
Jacine,
Your last patch does not address one important point. If a user has permissions to edit or delete one node, but not another the formatting of your table can very easily be messed up. The reason being is that your check decides row by row to to output the number of td tags and makes no adjustments in either appending td tags necessary or setting the colspan attribute; this will result in rows with varying numbers of cells.
I decided against the functionality with yoroy as the loop needed would be very complex to reside in a theme hook.
See comments #107 and #108 above.
#207
ok, merged both patches, and can be please be done with this issue now? :P ;)
#208
Incorporated #206. Now for real!
#209
Leaving blank table cells is sloppy at best, and I consider it a bug. It's bad enough that we have it in the blocks table, when there is no option to delete a block. It really pains me to see this sloppyness added to new functionality, especially when we do not technically need to use a table. It seems no one is really interested in fixing that here though, so I'll drop off.
One last note re: #208, FYI - my screenshots do not reflect removing padding from the block.
#210
I committed #208 but we can continue to improve the generated HTML per #209. Jacine -- don't give up!
#211
@Dries & @sun - Thank you for your encouragement. It means a lot. :)
I have been working on this and have a fix to propose. Unfortunately, during my time working on this today, I found quite a few more issues that I am bringing up in this patch because they are related and what I'm seeing really concerns me.
Before and after screenshots:
#212
Looks like a winner to me Jacine, nice work! I'll let someone more intimate with the themeing in D7 set it to RTBC though.
#213
This looks good to me.
One thing I'd like to see us work on as a follow-up patch is renaming 'more' to 'More' and standardizing some of those links. For example, I think 'Show all content' is technically not 100%.
#214
Actually, this needs work. I totally forgot but there is a theme function for the more-links that this should be using. I'll fix this patch and change rename "more" to "More" in the theme function.
http://api.drupal.org/api/function/theme_more_link/7
#215
Here's an updated patch that uses theme_more_link(), and modifies the theme function to read "More" instead of "more."
#216
Some of the changes make sense, but:
+++ modules/node/node.css 15 Feb 2010 20:24:43 -0000@@ -43,3 +43,16 @@ td.revision-current {
+#block-node-recent td {
...
+ padding-right: 8em;
+ position: relative;
...
+#block-node-recent td ul.links {
+ position: absolute;
+ right: 0;
This is a neat idea, but this will make the edit/delete links overlap with the node title, in case the links are localized into another language (which may lead to longer link texts).
Doesn't work out.
+++ modules/node/node.module 15 Feb 2010 20:24:43 -0000@@ -2157,22 +2157,35 @@ function theme_node_recent_block($variab
+ 'attributes' => array('class' => 'edit'),
...
+ 'attributes' => array('class' => 'delete'),
Love these extra classes.
+++ modules/node/node.module 15 Feb 2010 20:24:43 -0000@@ -2157,22 +2157,35 @@ function theme_node_recent_block($variab
+ $action_links = theme('links', array('links' => $links));
+
+ $row[] = theme('node_recent_content', array('node' => $node)) . $action_links;
$rows[] = $row;
This means that the table would be superfluous (a single cell on all rows).
However, also bad UX: In case you do not have "edit" permissions for a certain node, the "delete" link will appear in the visual column of edit links.
Powered by Dreditor.
#217
The table was superflous, but better than having empty cells IMO. I tried to test this with some other languages, and it's true, it will not work out. The padding would have to be increased too much which brings us back to the same sad place as having the empty cells in the first place. :(
Another thing I noticed is that the more link, which links to "admin/content" needs to go away for users without permission. This patch reverts back to initial row method, adds classes to the cells, and fixes showing the more link to users without permission.
#218
Looks good to me.
#219
Awesome, thanks for this clean-up! Committed to HEAD.
#220
Automatically closed -- issue fixed for 2 weeks with no activity.