Usability Issue: Post Information Settings
gopherspidey - May 12, 2007 - 06:17
| Project: | Drupal |
| Version: | 7.x-dev |
| Component: | usability |
| Category: | task |
| Priority: | normal |
| Assigned: | Jody Lynn |
| Status: | patch (code needs work) |
Description
I just spent 10 mins looking for the check box that turns off the Post information on a content type. I had remembered that I saw it some where, but could not remeber where.
I feel that it should be moved from the Themes configuration (Where I finally found it) to the individual content type page.
I believe that the placement on the themes page is because prior to Drupal 5 the content types page did not exist (I might be wrong on that).
The reason that I suggest that we move it there is because that is the page the I naturally when to look for it.
If I would understand the forms API better I would submit a patch, but currently I am lost.
#1
Ok it was easier that I first thought.
#2
Of course form API is easy and I believe this is a useful patch.
#3
A useful patch, but is broken right now. My cut and paste that I did last night was not good. I am looking at it right now.
#4
#5
Subscribing... I'd like to re-roll this patch, because I'm answering the question about where this setting is for people all the time.
#6
I would not remove it from themes configuration, because you will get the same user questions again.
So I tried to reroll it without touching the admin/build/themes/settings page.
The patch works, but I don't know what to think about it. (About modifying theme settings inside content type settings)
And I found new php notices in theme.inc in function theme_get_setting(). Or am I making something wrong?
notice: Undefined index: in ...\includes\theme.inc on line 887.
notice: Trying to get property of non-object in ...\includes\theme.inc on line 899.
notice: Trying to get property of non-object in ...\includes\theme.inc on line 908.
and, by the way (i did not tested, but) the previous patch do not work, because it save the value into simple variable toggle_node_info_$type, but the node display (and others) tries to read it from variable theme_settings.
#7
I think it would be better code if we redid this variable completely. It should now be a node type variable rather than a theme variable.
Also I think the setting should be removed from its old (bad) location completely. This change will just be one of many new things to get used to in a new version and I don't think we should maintain pieces of legacy problems in an effort to avoid retraining.
#8
I absolutely agree with Lynn. We should make a clear move here, and yes I think that it would be a right move. I've also been searching that setting and eventually found it where I least expected it.
#9
quick search for toggle_node_info in drupal core
theme.inc (2 matches)
system.admin.inc (2 matches)
system.module (2 matches)
default.profile (1 match)
chameleon.theme (1 match)
it is not too much, but I think the requested change is big enough for drupal 6 at release candidate state
so marking it for 7
#10
Yeah, that might be better.
#11
Marked http://drupal.org/node/138629 as duplicate.
Despite being marginally older, you are already on the same path regarding changing the variable location that I was considering in my patch.
Pasqualle, do you want to roll a new patch with the variable change? or may I?
#12
we should use a checkbox instead of radio buttons, since it is a single on/off option.
Attached example has different wording as it was from the patch I was working on in the other issue, but you get the idea.
#13
Here's a patch. It adds a 'Display Settings' fieldset to the content type page. Patch actually removes a lot more code than it adds, since the old way of making this a theme setting was a mess.
#14
I think, it would be better to store it into 1 variable as array, instead of using 1 variable per node type.
#15
Hmm... honestly, I don't see what benefit you'd receive having it all in 1 array. This way makes it easier to determine if the post settings have been set for a specific content type by accessing the variable directly. However, my hunch is that it would be better to still determine post info settings using theme_get_setting() for uniformity's sake, unless other theme related variables are being accessed directly with variable_get(). It just seems like it'd be better to keep coders using a single function instead of using theme_get_setting() for some cases and variable_get() for others.
#16
Node_options (default workflow options) is already one variable per node type. A different solution would be to add this variable into that variable's array, if you are trying to avoid extra variables.
The problem with storing this variable as an array containing all node types is that you then need to look into changing node_type_form_submit which has rules about what to do with node type specific variables when a node type's name is changed.
#17
The only place where we have to do variable_get is in template_preprocess_node and I don't see that it makes any difference. I think it fundamentally makes more sense for this setting to be a node type variable rather than having anything to do at all with theme_get_setting.
#18
Did not apply smoothly to HEAD, but works. Also, this patch clears this setting on Theme configuration page, which makes the page cluttered.
Is it worth changing theme configuration page to http://drupal.org/files/themes_0.png ?
#19
Hey meba, I will reroll it for HEAD. Not sure what you are saying about the theme configuration page though - is that link to the right file?
#20
Ok- your attachment is the right image. I think that is a good change, but will also look into the addition of custom theme settings and where that goes in that layout - some themes add extra options and we want to make sure those go in the right place too.
#21
Marking to needs work for the reroll. This is a really, really nice patch to fix something which is really irritating.
Also, hiding a node type setting in theme administration is a bug.
#22
OK, I rerolled it for HEAD and moved things around on the theme settings page as per meba's suggestion. However now things are not quite right visually on the settings page (the placement of the submit buttons and difficulty fitting the upload bars in the fieldsets).
Any more layout suggestions? The patch also just needs general testing.
#23
Personally, it would seem cleaner and more in line with the rest of Drupal's forms to just get rid of the float. It was handy since the other box was so narrow, but I don't see any reason for these logo forms to be to the side. It makes for an awkward situation now in your screenshot where the submit buttons are to the left of some actual form fields.
#24
Patch works, but I agree with rszrama that there's no reason to change the layout of the theme settins.
Since the name of the variable that we use to store this setting is changing, the current settings are lost (all content types are displayed with the post information until changed), an upgrade path should be provided.
#25
OK, I got rid of the floated layout on the theme setting page, which is definitely the simplest way to go.
I also added an upgrade path to convert the old variables to the new ones. The upgrade path is untested.
I did test the patch on a fresh install and it does properly set the page content type to not display post information, so I think what's left is to test the upgrade path from D6 to HEAD after applying the patch.
#26
Lynn, did you mean to attach a patch?
#27
Woops - thanks.
#28
No longer applies. :( Also, we're now up to update 7010.
I want to fix this so that I can implement #216961: Make author/submitted by separate toggles
#29
#30
Here's a re-roll, I think. Untested.
#31
Fixed a few coding standards issues I found on visual patch inspection.
#32
Patch looks good, less code too. There's no tests for this, but it doesn't break any tests either, and I checked it manually.
#33
webchick reminds me we need to test the upgrade path.
#34
The update function was indeed causing errors.
Updated patch changes
$typeto$type->type(because node_get_types() returns an array of objects), fixes a typo in a variable name and formatting in the update message. The update is now running cleanly, and it works!#35
Excited to see this moving again! Will also test the upgrade path myself this week.
#36
#37
I like this. Note that there is a:
+ * Change the theme setting 'toggle_node_info' into a per node variablesmall style violation with the messing period that could be fixed during a re-roll, perhaps for another change if one pops up.
#38
From reading I can't tell anymore if this patch is now about
1. moving post info settings to the content type form?
- I agree that's a much more logical place for it
2. redesigning the theme settings page?
- Please stay within the scope of the issue and only fix any layout issues caused by removing the post info settings there.
I'd like to see a screenshot of the whole content type form. Where is the post info checkbox placed?
Just a friendly bump from ux-team! :-)
#39
@yoroy: Yes this patch is about #1. We decided to just remove the section of the theme settings page which previously held the 'post information settings'. The layout issues we were discussing were precisely due to layout issues caused by removing the settings but I think we're all resolved on that.
The content type form will have a new collapsed fieldset from this patch called 'Display Settings'.
This patch is currently on hold because its upgrade path requires testing which is being prevented by critical bug #303889: Impossible to update D6 -> D7.
After we get the functionality reviewed would it be easier to deal with to have a new issue for any further layout refinements to theme settings or content type pages?
#40
This still looks great, shame we can't test it. Also it needs a re-roll for system_update_N numbering.