Add a 'recent content block' for use on the dashboard
| Project: | Drupal |
| Version: | 7.x-dev |
| Component: | node system |
| Category: | task |
| Priority: | critical |
| Assigned: | CodyCraven |
| Status: | needs review |
| Issue tags: | dashboard, Favorite-of-Dries, Needs usability review, Novice, UBUserTesting2009, Usability |
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 | Re-test |
#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.