Site Information Improvements

I'm struggling with this change, so I'd love to get the UX team's take.

In overlay the string "site Information" gets duplicated.
I also added a better screenshot to see the difference.

Yup looks good, there are some minor details I am not sure about - mostly because I am not fully understanding all the diffrent things going on in the comments. Going from just aspilicious - putting some fieldsets in makes sense here.

Looking at the screenshot in #14 make me realize that grouping these settings under "Site information" is rather broken. Clearly, "Cron" is not "Site information". Neither is "Error pages". We got it all wrong.

Personally, I think it would make sense to break these 4 fieldsets up in 4 pages. In my opinion, "Cron" should go under System, "Error pages" should go under "User interface", etc.

Thanks adam! I think you're on the right track, and I'd work towards making the tests pass at this point.

Let's leave 'Front page' where it is for now, and then we can discuss the location of 'Front page' once we agreed to break up the current page. We could make it a follow-up patch in this very issue.

My personal take is that it could fit under structure once the actual 'Front page' settings have been broken out into its own page (but I'd still like input from more people). Until we break up this settings page in multiple pages, we can't move it. It just doesn't make sense to move 'Cron' and 'Error pages' under 'Structure'.

So let's get these tests pass first ... :)

Wait, what happened here :) ? I thought we wanted to avoid having single page configuration. And I do not get all this talk about putting stuff in Structure neither. Can we have proper screenshots? Also we might got it all wrong, but please explain me clearly why moving all these settings around will make it more discoverable?

Also Error pages does not belong in User interface, its a system level setting not something that enhances or changes the interface.

Bojhan: I believe I clearly explained the problem. Have you read this issue carefully? Happy to answer specific questions ...

@Dries I understand relocating, I just don't see the advantage of breaking this up in 4 separate pages.

Subscribe, will review soon.

@adam.hastings None of this belongs in Structure, then again I feel we are discussing the fundamentals of this patch after the fact.

Lets first review, why do we need to split these configurations up? Al right, error pages doesn't really belong under Site information and neither does cron. But do they deserve their own page? What would be the downside to this change, it will create more pages making the functionality more scatted and adding more clutter. To the advantage is that these links should be more discoverable, I am honestly questioning this late in the stage of development we should be making this kind of change - where the actual win will go completely untested and uncertain.

I agree with adding the fieldsets.

Cron probally Development, and Error pages System? Yhea I am still wondering about error pages, there is few advantages to optimizing such a infrequently access functionality while increasing clutter which will impact all efficiency of the configuration listing./

subscribe

Last patch looks good.
403 and 404 is errors, but its not development errors. Even if you write "most clean" code in the world you will have this errors.
I add little change to patch, added t() to descriptions and titles.

Something strange happened with tags. Roll back them.

+++ modules/system/system.admin.inc	25 May 2010 01:57:26 -0000
@@ -1500,40 +1500,83 @@ function system_ip_blocking_delete_submi
+    '#title' => t('Site Details'),

Should be 'Site details' with small 'd'.

+++ modules/system/system.admin.inc	25 May 2010 01:57:26 -0000
@@ -1500,40 +1500,83 @@ function system_ip_blocking_delete_submi
+    '#title' => t('Front Page'),

Should be 'Front page'.

+++ modules/system/system.admin.inc	25 May 2010 01:57:26 -0000
@@ -1500,40 +1500,83 @@ function system_ip_blocking_delete_submi
+    // Set to default "node"

Should end with a point.

+++ modules/system/system.admin.inc	25 May 2010 01:57:26 -0000
@@ -1550,33 +1593,84 @@ function system_site_information_setting
+    form_set_error('site_403', t("The path '@path' is either invalid or you do not have access to it.", array('@path' => $form_state['values']['site_403'])));

'@path' looks incorrect -- why not use %path?

+++ modules/system/system.admin.inc	25 May 2010 01:57:26 -0000
@@ -1550,33 +1593,84 @@ function system_site_information_setting
+    form_set_error('site_404', t("The path '@path' is either invalid or you do not have access to it.", array('@path' => $form_state['values']['site_404'])));

'@path' looks incorrect -- why not use %path?

+++ modules/system/system.module	25 May 2010 01:57:28 -0000
@@ -957,13 +959,31 @@ function system_menu() {
+    'title' => t('Error Pages'),

Should be 'Error pages'.

Powered by Dreditor.

Is the summary in #33 still correct? This discussion is all over the place, a lot of it sounds like change for change's sake and I'm seeing very little reasoning behind the proposed changes. I can test the patch but have no idea what to look for.

Summary:

• Moves Error Pages out of Site information into its own page under Structure
  • Adds validator for error pages paths
• Moves Cron out of Site information into its own page under Structure
  • Adds "Run cron" button, as well as time cron was last run, to Cron page
• Creates fieldsets for Site Information page (Site Details and Front Page)
  • Front page path is no longer a required field, left blank defaults to "node" so that users never have to type / see the word
• Updates menu descriptions and help text
• Updates system.test to recognize new paths

Corrected all errors mentioned above + some more+remove empty whitespace.
I add screenshots of old and new version of configure page.

Can you put the @ingroup behind the @see.

+/**
+ * Validate the submitted site-information form.
+ */

==> Validates

Submit ...

==> submits

why? ==> http://drupal.org/node/1354

fixed version.

I think this is a nice improvement. More logical and things are easier to find now. @yoroy -- any thoughts?

Yes, if you put things directly on the configuration page they'll be easier to find. But we can't do that for all links, so we have to ask ourselves if *these* things really need to be easier to find? They are mostly one-time setup things and with this patch they get a very prominent spot on the configuration page, *always*. Especially the error pages one. There's no real problem with never filling these ones out since there's an ok default behavior. Currently they are shown together with the one for front page, no? People go hunting for that one far more often, which makes the ones for error pages quite discoverable, too. Same for cron, it has an ok default out of the box, though it's one you're likely to look for a bit more often. So, could see the case for cron but not really convinced we should do this.

notes on the screens:

I agreed that all this options mainly the one time to set(may be cron not). But almost all fields from "Site information" page user fill when running install script first time. At this point user may not know what error page he should use on 403 and 404 errors, he simply not think about it(not developed it yet). The same applyed to cron. User may not know what is it.
So user must find this options when he already done some job about site and decide what he need.

Ok i need to set up custom error page. It is definitly configuration section. Then where it is? If i see the Site information section i personaly never think that error pages is about infomation of site. Especialy if i open page with site information i could imagine situation when user see first field and think that he already filled all parameters here at install time and close it. So we try simplify access to this options. Mainly this will be used by disigners who don't know deep structure of drupal. Besides cron options and error page not actually Site infomation.

Description of "Cron page". What about: Manage scheduled jobs, check for updates, index site content, etc. ?
Scheduled jobs wiil sound familiar for not linux users.

Status report page is mainly the result of cron job. So it may be good to have link to that page here. Any idea what best fit there?

Outer fieldset will be fixed.

For cron, what about: "Manage automatic site maintenance tasks." It's maybe not 100% accurate, and there is some danger of confusion with maintenance mode, but it should be clear enough to get the point across. There probably should be a 1 or 2 sentence explanation at the beginning of the page of what cron does, to help users figure out what they should set the interval to.

"Manage automatic site maintenance tasks" I like this.

Next version.
About fieldset for whole form. On similar pages like "Performance" and other with fieldset there is no fieldset for whole form. I think there shouldn't be too.

Config page: http://skitch.com/yoroy/d8844/configuration-d7

Site information page: http://skitch.com/yoroy/d885g/site-information-d7

Cron page: http://skitch.com/yoroy/d885x/cron-d7

Error pages page: http://skitch.com/yoroy/d885u/error-pages-d7

Looking at this and clicking around a bit I really don't think this is going to improve things. Especially the error pages on a separate page is unnecessary. Why does this need its own page when even if you never change anything for these pages nothing gets broken? Move these back to their own fieldset on the Site information page.

I'm still undecided about the separate cron page, but in its current state the text still needs work:

needs work that is.

Good. This patch implements the @yoroy version as described at #55.

#57: site-information-settings-7.patch queued for re-testing.

I think yoroy's suggestion was to get rid of the first fieldset and leave the seconds, not to put them both in one.

I personally think that the first fieldset should stay and the second one should go, as that would be a bit more consistent with the 'Clear caches' button on the Development page, which is AFAICT the only other action button on a settings page in Drupal.

Either way, only half of the stuff should be in the fieldset.

There's also the site re-indexing button on the search settings page.

...which is also in a fieldset. I guess we have a pattern?!

to #60 I tried this too. Pesonaly i don't like both version(use only bottom filedset or only upper one). If you use only one fieldset other part of information looks like fly somewere. It's ugly. Put whole page in one fieldset makes page feel more like in one entity.

Use only the bottom fieldset.

The guideline is:

No fieldset around the first part of a form if:
1. It is the main interaction on the whole page (80/20 rule): Do people come *mainly* to this page to perform the action in the first part?
2. Subsequent parts of the form are related to the first part ("advanced settings").

Both are the case here. See also Appearance page for example, where the main interaction of choosing a theme is not wrapped in a fieldset and the setting for administration does have a fieldset

(The 'empty cache' button on development page has a fieldset around it because nr. 2. from above doesn't apply. Even if it's the main interaction on the page, all subsequent settings below it are not directly related to it, so 'empty cache' gets a fieldset itself as well)

Thanks for review. Patch with latest fix #64.

#65: site-information-settings-8.patch queued for re-testing.

Patch still applies and seems to work.

@Dries Guess you can commit then?

Committed to CVS HEAD. Thanks.