Posted by Noyz on September 6, 2011 at 5:14pm
15 followers
| Project: | Drupal Commons |
| Component: | User interface |
| Category: | feature request |
| Priority: | major |
| Assigned: | ameenia |
| Status: | closed (fixed) |
| Issue tags: | Usability |
Issue Summary
The current treatment for node links is a mess. Here's a solution that should fix this...
Comments
#1
This would be a huge improvement. Thanks, Noyz!
#2
I am working on a new theme for Commons, most of this is accounted for in it. I hope you don't mind that I will probably use this type of layout.
#3
@Noyz I think this is a phenomenal improvement. Clears up a lof of the clutter and is much more intuitive.
#4
Subscribing.
#5
Adding the usability tag.
#6
+1
#7
I posted a module called Node Link Style that might help with this. It allows you to modify the wrapper elements, CSS classes, and list types of node links. Therefore you can wrap the "Add comment" link in a div outside of the list to display it in the image below. You can also group similar links like the subscriptions into it's own list to float right to be displayed like the image.
#8
Awesome stuff, Chris!
I'm marking as needs review. I look forward to checking this out!
#9
@Noyz
While this looks like a huge improvement, one thing I'm starting to find with the Commons themes is that they seem to break node away from comments a bit too much. Drupal.org is a good comparison with the 'Add comment' buttons floated on the right hand side and node flows logically into comment. With too many links and graphic between node and comments it both makes comments look too separate, and worse can mean users don't even see them.
@cpliakas for this reason though your module could be a godsend for tweakers.
Similarly, I think in terms of the comments, having the user information on the left is wrong... That's where our attention is automatically drawn, and the most relevant content should be there i.e. the actual post. While looking nice, the comment bubbles should be on the right, again it works well on drupal.org on the top of a comment, but I do like the bubbles and the only other place for them would be on the right.
#10
subscribing - great discussion!
Early days for this module but I'm using it with some mods to sort out the subscription/notification mess of links: http://drupal.org/project/itweak_links
#11
Hm. Didn't know about that project. My module allows for configuring everything via a GUI with exportable settings, but has nothing predefined. iTweak Links has no GUI, but comes with some predefined styles. Looks like if we collaborated we could come up with something cool and flexible OOB. I will post an issue to see if this is possible. Thanks for pointing this out!
#12
@cpliakas : Let me know if you move ahead with this as I'd be interested in helping out. I'm pretty stretched at the moment but happy to help if I can.
#13
@Crom with itweak_links, do you have a solution for stopping the subscribe dropdown from falling behind the right sidebar? Whatever I do with z-index doesn't seem to work.
#14
Hi lighsurge - I don't have that problem...strange. But in similar circumstances I've found that overflow:visible on the container div sorts it out.
Hope this helps,
Crom
#15
Here is a patch to improve node links. It uses node_link_style module from cpliakas to replace default style. The patch adds a new feature: commons_node_links. It contains CSS and overridden theme functions.
#16
Updated patch
#17
Applying this patch and enabling commons_node_links makes all links disappear on a discussion node.
Is there a context or other configuration that's supposed to be exported here?
#18
That's right, context for node_link_style should have been exported in feature but it's missing. I will update the patch.
#20
Here is a new patch for node links:
- context problem fixed
- feature added to commons profile
- UI enhancements
#21
@laurentc, there is no patch attached to the previous comment :).
#22
Forgot to attach... Here is the patch.
#23
Marking as "needs review".
#24
Awesome - It is exciting to see this starting to look like the comp!
I've noticed a few functional and code-level issues. For some, it would be great to get input from cpliakas.
Function issues:
- The "Bookmark this" link should be in dropdown per the comp. For me it appears next to the comment button.
- I'm seeing an overflow issue where the node links mouseover is truncated by the lack of overflow on the main content div
- This feature should be enabled for existing sites via a hook_update_N() implementation
Code issues:
- I notice you are doing a regex to extract the href from the node links. My initial thought was that this shouldn't be necessary, since node links are split out into a keyed array and passed to hook_link, which node_link_style implements.
<?php/**
* Process variables for node.tpl.php.
*
* @see template_preprocess_node()
*/
function node_link_style_preprocess_node(&$variables) {
if ($plugin = context_get_plugin('reaction', 'node_link_style')) {
$variables['links'] = $plugin->execute($variables['node']->links);
}
}
?>
This makes me wonder if this patch implements the plugin in the way cpliakas was expecting when he wrote the module. It would be great to get input from him.
- Similarly, when the plugin is executed seems like the appropriate time to add css/js to the page, rather than in hook_init. Again, it would be great to get input from cpliakas on how he intends for plugins to be used.
Less important:
- Is it possible that hook_element and the Form API would be a more appropriate way to define commons_node_links_custom_group()? This feels like a non-critical and low priority point to me, since we're not actually creating an element that will be submitted via POST.
#25
I should have posted a screenshot:
Here's what I'm seeing in Chrome 15.0.874.120 on Mac OS 10.7.2 when viewing a Discussion node. It looks similar in FireFox 8.0.
Sadly, I'm not sure how to reproduce the overflow issue I was having before. I think it had to do when there was content below the main node. If we can't reproduce, then let's disregard that bit of feedback.
#26
"Bookmark this" hasn't a correct link format. It doesn't send 'href'. Text is already formated. (regex code is to fix this problem to answer you previous question). But it should anyway appear in the dropdown. This is strange.
Could you provide a db dump? I would need it to check node_link_style configuration and debug the problem.
Thanks,
Laurent.
#27
The commons_node_links feature is not overridden, which suggests that the config is the same as in the 'dev' branch.
I'm happy to provide a db dump, but first can you confirm that you don't get the same behavior as me when you have an existing 6.x-2.x site, switch to the dev branch and enable the commons_node_links feature?
#28
I tried again with a fresh install of 6.x-2.x and switched to the 'dev' branch, and wasn't able to reproduce the "bookmark link" issue. So, looks like that was something weird with my installation. Sorry about that!
However, with the sample discussion node that ships with Commons I once again see the overflow issue:
As an alternative to adding CSS in hook_init(), I suggest doing this in commons_node_links_custom_group() and in function execute($links) in node_link_style_reaction_apply.inc.
#29
Also, to closely match Jeff's screenshot, I think we need to remove "Groups: Our Community" from the bottom of the page.
#30
One more comment: I understand now why you are using _extract_href() -- I suggest renaming that function to _commons_node_links_extract_href() and adding a code comment explaining why this is necessary for some links, eg: "Flag module toggle links don't set the $link['href'] value and instead use $link['title']."
#31
I think this is a great start, but I am marking it as "needs work" for the reasons outlined below:
The first reason is that the revised logic of the Node Link Style module is a little confusing. There is a new "Group" textfield which is unclear as to what it does, but it appears that if you enter a value then it effects the logic of how the node links are styled. If you don't enter a value then it uses NLS's normal theme hooks and takes into account the settings you added vie the GUI. If you do add a GUI, then a second "dummy" theme hook is invoked and the settings passed through the GUI are ignored. It is the responsibility of the custom module to implement hook_theme_registry_alter() to point that theme hook somewhere else.
I think that laurentc discovered an important point in that the Semantic Views style approach where we add wrapper elements and classes vial the GUI won't work for every use case, and you will need to implement a more custom display in order to achieve more advanced results. Therefore I think we should add the concept pf display plugins to Node Link Style that allows for different types of styles to be selected. We would have the Semantic Views-style plugin, the Jeff Noyes style plugin rendering links with the markup that laurentc created, and any other third party plugins that are available. The workflow from a site builders perspective would be to add a link as a styled link, select the display plugin, and then configure the plugin specific options. Below is a flowchart further illustrating this idea.
The wireframe below outlines the changes to the UI.
This would eliminate the need for hook_theme_registry_alter() and allow for different displays to be added in different modules or in Commons directly.
#32
As per the conversation with ezra-g and laurentc on IRC, the simplest solution is to ditch Node Link Style and write a small custom module implementing hook_preprocess_node() which will override $variables['link'] with the markup that laurentc created in the patch posted in #22 above.
#33
Thanks, cpliakas for documenting the summary!
This work is happening at http://drupal.org/sandbox/ezrag/1344006.
#34
The overflow issue seems to be caused by fusion core style.css and relates to line 189
.row, .nested, .block {overflow: hidden;
}
This css isn't getting properly overidden by the grid system and the content of the dropdowns gets clipped.
Setting it to visible in origins local.css seems to work and I've had no 'visible' problems with that.
#35
Looks like the post by lightsurge at #1346066: Node submitted meta information is a bit messy seems to be attacking the same issue. Posting for reference.
#36
Here's a patch revision using tidy_node_links module (http://drupal.org/sandbox/ezrag/1344006)
#37
This looks good!
I tested on a fresh upgrade an install with Commons_origins as the default theme and the widget was enabled in both cases. Note, the out-of-the-box styles provided by this widget work best with Commons_origins but could be extended to work with the other themes.
I filed 2 followup issues related to this one:
#1346546: Make overflow visible for sidebar blocks & main content area
#1346544: Move node group listing to the top of the node, clarify from tags.
I also corrected some minor spacing issues with the conditionals in this patch.
I won't RTBC my own patch, but I think this is ready to go :).
#38
#39
Thanks you for the review. I've committed.
#40
Here's the commit, for reference: http://drupalcode.org/project/commons.git/commitdiff/ac00ae8?hp=b58623ff...
#42
Automatically closed -- issue fixed for 2 weeks with no activity.
#45
Re-closing the issue. Reporting ameenia as a spammer.Removed at #1388538: User ameenia posting spam comments.