| Project: | Drupal core |
| Version: | 8.x-dev |
| Component: | node.module |
| Category: | task |
| Priority: | normal |
| Assigned: | rootwork |
| Status: | needs review |
| Issue tags: | #SprintWeekend, Needs issue summary update, Needs usability review, Novice, SettingsAPI, UBUserTesting2009, Usability |
Issue Summary
Motivation
Some users go to the content section, rather than the structure section, looking for options to edit content types.
Proposed resolution
On the node/add page, add links to edit the content types as well, for users who have the right permissions. This provides another way of navigating to admin/structure/types/manage/[node-type]
Remaining tasks
Summary of API changes
Needs review
User interface changes
Adds links to admin/structure/types/manage/[node-type] for each node type on the node/add page
Screenshot of UI before patch:
Latest screenshot of UI after patch (from comment #50):
Related Issues
- #1440678: Add a "Create content type" link to the content listing page and/or the add content page
- ?
Original report by catch
Many users go to the node/add and node/edit forms looking for settings (submitted information, input formats). We should consider adding links there to the admin screens for those form elements when users have the relevant permissions.
Comments
#1
Fixing tag.
#2
This might be good for the novice queue.
#3
Sorry, I screwed up the tags.
#4
Hey, I'm completely new to Drupal development, but this looks like a great starter project. I'll see if I can get a patch done by tonight.
- Lucas
#5
First open source patch!
The method I use to do this is arguably a little hack-ey, but because of the way that drupal handles the node/add page, its the easiest and most efficient way. The node/add page stores the content types as menu items, and displays them back without any knowledge of the content type its describing. For me to provide a link to edit the content type, I have to reverse engineer the URL that goes with the menu item.
#6
#7
The last submitted patch failed testing.
#8
Fixed patch. Relative filename problems...
#9
Not sure why this failed to attach the first time
#10
The last submitted patch failed testing.
#11
Fixed errors with declaring variables. That will teach me to run my server in E_ALL...
#12
Please, please be the last try. I have to manually change the file path because diff is being mean to me.
#13
#14
Hey lucaswoj, your code is not adhering to the Drupal coding standards. If you look at the patch, you'll see that you're using tabs instead of spaces. It would be great if you could re-roll the patch with Drupal's coding standards in mind.
#15
I'm sorry, I thought my IDE was already configured for spaces... I guess I've never really worried about it before.
For the 5th time, here we go....
#16
this worked for me.
#17
Thanks for your patch. It almost works as advertised - here are some remarks:
Inconsistency:
The patch indicates that line 25 replaces line 23, but they're identical so the patch should not indicate changes.
23: - $output .= '<dt>' . l($item['title'], $item['href'], $item['localized_options']) . '</dt>';24: - [...]
25: + $output .= '<dt>' . l($item['title'], $item['href'], $item['localized_options']) . '</dt>';
Markup error:
On line 28 of your patch you're adding:
28: + $output .= '</dt>';but you really wanted to close the DD element opened on line 26, so line 28 should be:
28: + $output .= '</dd>';Usability:
The patch adds an 'Edit Content Type' link to the end of each Content Type definition on /node/add. However I only noticed that link after clicking around trying to find the changes introduced by the patch.
Wouldn't it make more sense to add the edit link next to the Content Type title? See attached screenshot.
Entities / wrapper:
27: + $output .= isset($item['edit_href']) ? ' <small>('.l(t('Edit Content Type'), $item['edit_href']).')</a></small>' : '';I personally don't like the addition of a non-breaking space entity and wrapping the "Edit Content Type" link in small tags. The entity clutters up the code and by using the small tags we make assumptions on how this link should be displayed.
Adding a meaningful css class name is more versatile and less imposing.
#18
Thanks for the thoughtful feedback jpoesen. I think this updated patch fixes all of your concerns.
#19
The last submitted patch failed testing.
#20
This has unfortunately missed the string freeze for 7.
#21
Yes, please.
#22
I was bored this evening, so here's a patch that brings #18 to D8, along with a clarification that the link is to edit the content type, rather than a link to bring up a view or something that would list all nodes of that type available for editing.
#23
The last submitted patch, content-type-edit-384150-22.patch, failed testing.
#24
Retry...
#25
#26
Coding standards
http://drupal.org/coding-standards
else ifshould be:
elseif$output .= $item['edit_href'] ? '<span class="node-type-edit">['. l(t('Edit content type'), $item['edit_href']) . ']</span>' : '';
Use one line dot concatenation or the operator .=
Same here:
$output .= '<dt>' . l($item['title'], $item['href'],$item['localized_options']);
better:
$output .= '<dt>' . l($item['title'], $item['href'], $item['localized_options']);#27
Better adherence to coding standards. (Sorry, got wrapped up in "80 characters per line".)
#28
+++ b/core/modules/node/node.pages.inc@@ -38,6 +38,12 @@ function node_add_page() {
+ //Generate links to edit content type if user has proper permissions
Based on other patches I've looked at recently, my understanding is that comments are supposed to have a space after the "//" and have sentence formatting, e.g.
// Generate links to edit content type if user has proper permissions.Note that I applied the patch (in D8) and am not seeing anything come up on the
node/addpage while logged in as super-admin. I cleared the cache and that had no effect either.#29
Drush wasn't working for clearing the caches so I changed the theme to clear the theme registry and now I see the links.
+++ b/core/modules/node/node.pages.inc@@ -58,7 +64,11 @@ function theme_node_add_list($variables) {
+ if (isset($item['edit_href'])) {
+ $output .= $item['edit_href'] ? '<span class="node-type-edit">[' . l(t('Edit content type'), $item['edit_href']) . ']</span>' : '';
I think it would be good to add a space between the content type name and the "[", e.g.
$output .= $item['edit_href'] ? ' <span class="node-type-edit">[' . l(t('Edit content type'), $item['edit_href']) . ']</span>' : '';#30
Also, there is no node.css anymore, that bit probably needs to be moved into node.admin.css
#31
Updated patch.
Also noticed that the links don't appear when using the Seven theme. Tried applying the patch, creating a new empty database, then installing Drupal, used another browser that had not accessed my dev environment, and still didn't see them on Seven. That *should* rule out caching issues.
#32
Ahh... there's a template.php override in Seven...
Here's an updated patch...
Also cleaned up the "if" surrounding the addition. When I first worked on this in December, I admit that I was just being introduced to the ...?...:... syntax at the time.
#33
I've cleaned up the patch so that it works with modern Drupal 8 code... the previous patch was suffering from some bit-rot. The attached patch is functionally the same as the previous patch.
I've tested the attached patch myself, but would love to see some additional review so that we can get this pushed through.
#34
Applied patch, looks good, works for core and custom content types.
#35
Hmm I think I'd probably place the permissions check directly in the theme function rather than overloading the node type objects. Also is the new CSS absolutely necessary in node.admin.css or could that be put in seven theme instead?
#36
This patch moves the CSS to Seven theme since it is not necessary to provide a default here. It also moves the call to user_access() and generation of content type edit links to theme_node_add_list().
#37
#36: content-type-edit-384150-36.patch queued for re-testing.
#38
Applied #36 to bc1da65 and rerolled it.
It seems to work.
#39
The last submitted patch, content-type-edit-384150-38.patch, failed testing.
#40
#38: content-type-edit-384150-38.patch queued for re-testing.
#41
#38: content-type-edit-384150-38.patch queued for re-testing.
#42
did a standards review. mostly looks good.
+++ b/core/modules/node/node.pages.incundefined
@@ -59,9 +59,17 @@ function theme_node_add_list($variables) {
+ // Generate links to edit content type if the current user has proper permissions.
+++ b/core/themes/seven/template.phpundefined
@@ -44,10 +44,17 @@ function seven_node_add_list($variables) {
+ // Generate links to edit content type if the current user has proper permissions.
comments should wrap at 80 chars. See: http://drupal.org/node/1354
#43
Applied #38 and rerolled it.
Hope it will work.
#44
Thanks @mahaprasad!
#45
screenshots embedded in the issue summary with before and after would help. Also it would help to update the issue summary using the issue summary template.
#46
Did a manual test of this patch. Works well but some minor issues.
1. There are brackets appearing in the output - see screenshot
2. The hover state for the 'Edit content type' links doesn't work - doesn't get an underline, as it should.
Also uploading a screenshot of the UI before to add to summary.
#47
Does this patch include any updates to node/edit pages? Couldn't find any, only saw changes to node/add. If there are proposed changes to the edit screen as well can someone provide more detail?
#48
Updated the patch from #43 to remove the extraneous brackets and give it the right span wrapper.
#49
Here's a new screenshot of the add content page with my revised patch.
It looks like the underlines on hover still aren't appearing. I'm going to try to track down what's going on in the CSS.
#50
Sorry, changing status back to needs work and embedding the new screenshot.
#51
Fixed the hover link & aligned the links in one line.
Please find the attached patch.
#52
Thanks @mahaprasad, @rootwork, and @pameeela! The updates make it much easier to understand the proposed change.
The proposed pattern seems pretty confusing to me, and it's different from any pattern we have elsewhere. We have two links that say "edit content type" with nothing to distinguish them aside from their spacial placement. I would have no idea which link I was supposed to click on when going to this page.
For this to work at all, I think the link texts would need to be more specific, like "Add new basic page" and "Configure basic pages" or something. Also, elsewhere when we have multiple operations related to one thing, we use the dropbutton pattern.
Tagging for the UX team to give feedback.
#53
Applied the #51 patch & tested screen shot is attached.
#54
Minor in style.css
\ No newline at end of file
#55
#56
@xjm (and useability reviewers), what if the pattern was more like the screenshot in #50? I actually thought that's what the previous patch was trying to create. I agree having the two links next to each other, with one in brackets, would be confusing.
What if they were on two separate lines, with the second line saying "Edit article content type," "Edit page content type," etc.?
Another option I could see is tacking it on to the end of the help text. So then it would say something like "Use articles for time-sensitive content like news, press releases or blog posts. Edit this content type." The edit link could be in parentheses, brackets, or italicized, although I think the fact that it's a link would probably set it apart enough.
#57
@rootwork @xjm I also think this is really confusing. I frequently do training for users new to Drupal and the concept of editing content vs editing content types is already hard to grasp. I think this blurs the line even further, by eliminating the content/structure split - which I see as a small hurdle to get over up front, but from there it makes a lot of sense.
I do appreciate this as an attempt to make things less confusing, so I am keen to see what the UX team thinks!
#58
@rootwork, #50 is exactly what I found very confusing. My review is about that screenshot.
#59
I'm reading the motivation in the issue summary:
I've seen this in Usability testing the multilingual stuff.
http://groups.drupal.org/node/285978 (g.d.o post). I have not gone through all the video to make notes yet. I'll come back and provide a specific example when I find it.
But, from memory, I think where they actually went was: admin/content (not the shortcut to add content):
And then tried the edit link for a particular piece of content when attempting to configure the content type in general.
on the add content page
But back to the add content page...
the text in [ ] reminds me of the [edit] [clone] [delete] links that used to show up when hovering over a view.
Consider:
1. What about using the word *structure*.
2. Also, since this is the add content page. We do not want the main emphasis to appear to be to edit the content type. (which it is I think in the screenshots previously posted)
3. Being specific about the links, so they are not identical.. there might be some code reusable from #1810386: Create workflow to setup multilingual for entity types, bundles and fields because we had to figure out the type of things when building the label for in input items.
a. Article[ Edit Article content type structure ]
b. Article[ Edit Article content type ]
c. Article[ Edit Article structure ]
d. Add an Article[ Edit Article structure ]
this shows a and b:
but I think my favorite might be:
I would mark this needs work for that. But I think we still want a Usability review to point this in an acceptable direction.
[edit: added width= on the img's because my retina display yields giant screenshots]
#60
added related issues section to summary with #1440678: Add a "Create content type" link to the content listing page and/or the add content page