Usability Issue: Post Information Settings

Of course form API is easy and I believe this is a useful patch.

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.

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.

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.

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.

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

Yeah, that might be better.

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?

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.

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.

I think, it would be better to store it into 1 variable as array, instead of using 1 variable per node type.

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.

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.

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.

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 ?

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?

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.

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.

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.

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.

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.

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.

Lynn, did you mean to attach a patch?

Woops - thanks.

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

Here's a re-roll, I think. Untested.

Fixed a few coding standards issues I found on visual patch inspection.

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.

webchick reminds me we need to test the upgrade path.

The update function was indeed causing errors.

Updated patch changes $type to $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!

Excited to see this moving again! Will also test the upgrade path myself this week.

I like this. Note that there is a:

+ * Change the theme setting 'toggle_node_info' into a per node variable

small style violation with the messing period that could be fixed during a re-roll, perhaps for another change if one pops up.

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! :-)

@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?

This still looks great, shame we can't test it. Also it needs a re-roll for system_update_N numbering.

I rerolled the patch, changed the update numbering, updated for a change in chameleon.theme, and fixed the code comment (comment #37).

I tested the upgrade from Drupal 6. It worked and all my 'post information settings' were preserved through the upgrade.

This makes so much sense. Yes please. Thanks for updating this Jody.

code looks good, the variable name could be better, but I have no objections. I like this patch..

Great! :)

However, I think we also need an update path.

...and THAT is exactly why I should not do cursory patch review after I spend all day in Views internals!

However, we *should* get some tests. And I grepped and there most definitely is NOT a .test file diff in that patch. ;)

update path? function system_update_7017() is in the patch

Doublecheck: http://drupal.org/files/issues/post_information.png is still how this will look?
Since this is basically an on/off switch, can we use a checkbox? Something like this:

[ ] Show <em>submitted by author on date</em> with content of this type.

I think we're trying to avoid the word 'post', no? Using a checkbox would remove a lot of redundant interface copy.

Also, hiding a node type setting in theme administration is a bug.

It doesn't seem so to me. Hiding them clearly has to do with theming IMO (Often theme A wants to show submission data while theme B doesn't), while the Workflow settings are about a node type's technical behaviour. I understand that a lot of users have trouble finding this checkbox right now, I am one of them, but I do not think moving it to the Workflow settings is the right solution.

@Xano: "Often theme A wants to show submission data while theme B doesn't" - that's not possible with the per-node type setting in HEAD, which is across all themes.

I agree with yoroy that this is a checkbox, and if possible it'd be good to move it out of workflow too - I don't think translation settings should be in there either, that took me a long time to find once.

that's not possible with the per-node type setting in HEAD, which is across all themes.

What do you mean, AFAIK HEAD uses is a theme-specific setting for this (like it should, IMO)

That is an old screenshot. In the last patch as I see, it is a checkbox an it is outside the workflow fieldset.

It is still possible to hide (or change) the node info in the theme.
here is a nice example how to do it: http://11heavens.com/displaying-the-content-type-name

Please, for the patch-impaired, post a screenshot of all parts of the UI that this patch touches if you can. Thank you.

It is still possible to hide (or change) the node info in the theme.

Ah, ok, thanks for the info :)

@xano:
The option is on admin/build/themes/settings but not admin/build/themes/settings/THEME - so it affects all themes with the same variable. We could even consider getting rid of 'global settings' entirely for themes after this.

Yeah, you're right. I just took another look at the theme settings; I didn't realise these settings were global. I'd think that configuring whether nodes display submission data would be a theme specific setting.

I think it makes sense to move it and to make them global: (1) 99% of all the sites have only one theme enabled and (2) this is about exposing information, not about theming information. If you want to disable it in one particular theme, you can do so in the template file. I think putting this option in the theme system was a mistake -- time to recover from that.

Here is a screenshot of the UI.

I can work on adding tests.

Thanks, Jody!

If you need any help with test writing, come ask in #drupal. We're here to help. :)

I added simpletests. The tests change the setting to display the post info for the page content type, create a page node, and assert that the post info displays or does not display appropriately.

Re comment #43 (variable name choice): I like sticking with node_submitted as the variable name because it's consistent with theme_node_submitted and $variables['submitted'].

Minor point, the description should probably read "Enable the submitted by Username on date text.", the "or disable" is not necessary and could be confusing, as it is a single checkbox.

Couple of issues with the patch, all relatively minor.

1. t('Node Post Info Display') -- we only upper-case the first word.

2. Please change 'info' to 'information'.

3. Spacing issues -- please don't use tabs.

Addressed issues in comments 60 and 61.

One very small thing, otherwise this looks RTBC to me: the new test class needs phpdoc for the class itself. Usually this is just a one line thing almost verbatim from the getInfo() description.

Added comment for the new test class.

(Note that the entire node.test is missing class comments if this is a concern for another ticket.)

Looks great! Gave it a run-through and looks good. This is one of my Drupal Pet Peeves, so I'm really happy to see it solved. Testing bot is happy, too! :)

Committed to HEAD!

Just in case you are enticed to keep improving this part of Drupal, I'd love to see #216961: Make author/submitted by separate toggles fixed too. ;)

Hooray! Thanks a lot Jody, nice one.

Fantastic, thanks for muscling through this Jody!

Tum te tum.. don't mind me. Just cleaning up some metadata. ;)

Hello,

I remember asking this question when I first started working with Drupal. And I remember suggesting that it be in admin somewhere instead of in themes. I'm glad to see others had the same idea and that it will be changed in D7.

Right now, I'm working on modifying (hacking) a theme on my local machine. Naturally, we want "submitted by" on for some content types and off for others. However, somewhere on my current live Drupal site I found a way to expose only name and date, not "time" of submission. I haven't been able to find this setting on the local site and cannot remember where I found it on the live site.

If anyone can tell me that would be great, but I mainly wanted to point this out for those of you working on the D7 core.

Thanks for all the work being done.

@2createwdrupal: There are no such settings inside Drupal admin UI. You need to override the search-result template in your theme..
http://api.drupal.org/api/file/modules/search/search-result.tpl.php/6

@pasqualle This is interesting and I will try it on the local set up and keep it in mind for future use. However, I somehow got "submitted by" to show not show "time" on my current live site without hacking core (that site was started with D6.1.).

I've done considerable hacking on the new theme: compared the "submitted by" sections of node.tpl.php and they are identical - I even cross pasted them, current to new, new to current and there was no change.

It may not be in the admin UI, but it is somewhere without modifying a core file such as "search-result.tpl.php" - I know I didn't hack core previously to get this "timeless" submitted by result. I'll find it eventually.

theming is not a core hack ;)

don't modify the original file. add the file to your theme and override (change it there)..

I was experimenting on a local machine set up and found that there is indeed a way - a simple way - to change the post-date-time setting and format. It is in admin/ Date and Time. Change/add a format. Select it in "medium" and it is done in the nodes that get post settings.

I was inspired by comments #70-74 to open a new feature request: #477170: Add Date and Time format select to Post Display Settings