Support from Acquia helps fund testing for Drupal Acquia logo

Comments

karschsp’s picture

Assigned: Unassigned » karschsp
karschsp’s picture

Status: Active » Needs review
FileSize
1.92 KB

Here's a patch.

Bojhan’s picture

screen?

karschsp’s picture

Here you go.

Default frontpage settings:

Frontpage settings before

Frontpage settings with custom frontpage:

Custom frontpage settings before

Default frontpage settings with patch applied:

Frontpage settings after

Custom frontpage settings with patch applied:

Custom frontpage settings after

Bojhan’s picture

Looks good to me, if there is a code review its RTBC from my pov.

attiks’s picture

Status: Needs review » Reviewed & tested by the community

not much code to review, just a block of code moved and one line added

Dries’s picture

This patch doesn't seem to apply. Asking for a re-test.

Dries’s picture

attiks’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
1.68 KB

Dries, was apparently a whitespace error, rerolled against latest dev

Devin Carlson’s picture

Assigned: karschsp » Unassigned
FileSize
3.45 KB

The patch in #9 looks good but it looks like it would require a page refresh to hide the "Number of posts on front page" options. I think it would be more user-friendly to have a checkbox that toggles between using the default frontpage and a custom frontpage using the states API.

I've attached a patch that creates a checkbox to toggle between a default/custom homepage and hides the extra fields depending on the checkbox's state.

Thoughts?

Status: Needs review » Needs work

The last submitted patch, front-page-settings-cleanup-1201592.patch, failed testing.

karschsp’s picture

Status: Needs work » Needs review
FileSize
714 bytes

Here's an updated patch that hides the number of posts field if the front page is set to something other than node.

karschsp’s picture

The patch in #12 actually fixes a minor DrupalWTF where, upon installation, the first time you visit the Site Information page, the "Number of posts on front page" doesn't show up until you hit Save, because it was doing:

'#access' => (variable_get('site_frontpage')=='node'),

which returns FALSE because the variable hasn't been set yet.

rather than

'#access' => (variable_get('site_frontpage', 'node')=='node'),
jenlampton’s picture

Looks like the patch in #10 was also adding a new checkbox for "Use default frontpage" which would determine weather the "Number of posts on front page" or "Default front page" field is shown. It's pretty slick. I'm also changing the name of the "Default front page" field, since it's now in conflict with the "Use default frontpage" checkbox.

How about we rename it to just "Front page" since it's no longer the default?

Also adding a space between front and page. :)

jenlampton’s picture

FileSize
28.63 KB
27.11 KB

I took some pretty pictures too :)

jenlampton’s picture

Status: Needs review » Needs work

Looks like the description text could use some work, on both fields. :/

jenlampton’s picture

Status: Needs work » Needs review
FileSize
31.53 KB
30.44 KB
3.24 KB

Okay, trying again.
font-page-default.png
font-page-changed.png

Title: Front page settings cleanup » North Face Jackets
Assigned: Unassigned »
Status: Needs review » Needs work

The last submitted patch, cleanup_front_page_settings-1201592-17.patch, failed testing.

attiks’s picture

Title: North Face Jackets » Front page settings cleanup
Assigned: » Unassigned
karschsp’s picture

OK, I've been trying to get the tests to pass for an hour and a half now and am ready to give up. The problem seems to be in lines 1402 - 1403:

$edit = array('site_frontpage' => 'kittens');
$this->drupalPost('admin/config/system/site-information', $edit, t('Save configuration'));

No matter what I do, the value for default_frontpage is always set to 1. I've tried:

$edit = array('default_frontpage' => 0, 'site_frontpage' => 'kittens');
$this->drupalPost('admin/config/system/site-information', $edit, t('Save configuration'));

and

variable_set('default_frontpage', 0);
$edit = array('site_frontpage' => 'kittens');
$this->drupalPost('admin/config/system/site-information', $edit, t('Save configuration'));

and I'm still getting the same 3 test failures. I'm sure I'm missing something simple, I've just been looking at it too long.
Thanks!

Devin Carlson’s picture

Status: Needs work » Needs review
FileSize
2.9 KB

It looks like it might be the default_value for site_frontpage. I think that can be simplified now that everything is handled by the form validation.

Status: Needs review » Needs work

The last submitted patch, cleanup-front-page-settings-1201592.patch, failed testing.

karschsp’s picture

Status: Needs work » Needs review
FileSize
3.31 KB

This should now pass all the tests. Just added:

variable_set('default_frontpage', 0);

In the setUp() for the FrontPageTestCase.

jenlampton’s picture

Status: Needs review » Reviewed & tested by the community

Works for me :)

catch’s picture

Status: Reviewed & tested by the community » Needs work

It looks like this is adding a variable, the sole purpose of which is to have a form element to trigger states from for the other settings? Where is 'default_frontpage' used apart from the form?

Seems like rather states could check the value of the variable, and hide the option if it != 'node'.

Also cross-posting #375397: Make Node module optional - this tries to remove 'node' as the default front page variable altogether, not sure how well these patches will play together.

karschsp’s picture

Status: Needs work » Needs review
FileSize
1.74 KB

Here's a re-roll based on catch's feedback.

catch’s picture

Status: Needs review » Needs work

Just realised another possible issue with this.

While you might not use the default front page for the front page, it's still available at /node. So hiding this setting would prevent changing the number of posts shows on /node.

jenlampton’s picture

But if you aren't using 'node' as your front page, then a setting defining the Number of posts on front page will really confuse people. What it really means is Number of nodes shown at /node

I'm not sure the right place for a "Number of nodes shown on /node" setting is under the "Front page" settings - if that isn't the front page anymore.

karschsp’s picture

Well, the whole Number of posts on front page discussion may be moot if #1210366: Per-bundle node listing pages, blocks, feeds. gets in. Then we could move that setting to each content types listing settings and this Front page setting could simply be, "Which path is the front page of the site?"

catch’s picture

Yeah I agree that's the wrong place for the setting, but that means we should move it somewhere else rather than hide it.

jenlampton’s picture

@karschsp We'd still need a "how many posts do you want on the front page" setting somewhere because /node isn't a type specific list, as mentioned in #1210366: Per-bundle node listing pages, blocks, feeds.

If we did decide to move the setting, where else would we put it? The only place that comes to mind would be with the 'Post settings' - and how on earth would someone hoping to change the number posts on their front page find that?

I think we should keep the current setting - with it's current language - under front page. When people want to adjust their front page, it seems silly to train them that those settings live in two (or more) different places. (And I still like the idea of hiding it once they change the path, too)

I think we are worrying *too much* about these people who are still using /node and the number of posts on it - after replacing their home page with something else. Are we sure these people exist?

This change as it stands now would improve the experience for the VAST MAJORITY of people using Drupal. .02 :)

jenlampton’s picture

And now for something completely different.

We have two different options, either the front page is /node, or it's not.
If the page is node, then we have an option of how many posts.
If the page is not node, then we need to know what it is.

So... what about just using a radio button?

I haven't gotten the descriptions to look quite right, they shouldn't be inline, they'd look nicer beneath... but here's a go at it.

Using radio buttons for front page settings

catch’s picture

Note that taxonomy module uses the same variable for taxonomy/term/n listings:


 if ($nids = taxonomy_select_nodes($term->tid, TRUE, variable_get('default_nodes_main', 10))) {

So it is not only sites that are using /node other than on the front page that would be affected by hiding the setting.

karschsp’s picture

@jenlampton I like the new approach. I wonder if the /node option should have the # of posts textfield in front of the text, so it would read:

10 posts, sorted newest to oldest

Where 10 is the textfield. Anyway, here's a patch that cleans up some whitespace issues and removes calls to dpm().

jenlampton’s picture

Issue tags: +D8UX usability

@catch, bah! okay - well maybe we can follow how #1210366: Per-bundle node listing pages, blocks, feeds. provides settings for each node-type, and do the same for each vocabulary. If we did that, then this setting would *truly* be a front-page setting, and would belong here :)

@karschsp, there are some HTML validation issues with that. The label tag comes first for radio options, and you can't have a form element inside a label :/ Thanks for the cleanup!

I think we need some standard like this in core. We currently have these horrid "leave blank for no title" or "enter <front> to link the front page" UI patterns everywhere in Drupal just because we don't have radios with additional inline options available. Webform and CCK have adopted this pattern, and it would be great to get it into core so that everyone has a standard way to solve this problem. (I'll happily replace all the "leave blank for no titles" if we can!)

jenlampton’s picture

Another issue which could add a setting to include / exclude that "Promoted to front page" checkbox #987242: The "Promoted to front page" checkbox doesn't do anything if the /node front page listing isn't used

And when we get done here, let's find a better home for these settings as per #1201580: Move Front page & Error page settings out of "Site information" and into their own respective config pages.

catch’s picture

One way to get around the coupling issue would be for node module to hook_form_alter() this additional setting into that page if it's set to /node. That doesn't get around the taxonomy issue, but it'd remove my concerns with further baking this into system module (in fact we'd actually be removing some coupling that way).

jmarkel’s picture

In the course of looking at #1798272: "Promote to Front Page" is confusing suggesting the removal of "Promote to front page" it occurred to me that "Number of posts on front page" was related, not least by its misleadingness.

With Views going into core, it might be worth considering the removal of node/taxonomy delivering multiple teasers (& related pagers), and let Views deal with that. It would certainly add more flexibility in configuring pages with multiple teasers, and would simplify node & taxonomy code. It's probably not common anymore to use /node as frontpage, or even straight-up /category/<vocabulary>/<term> for paged taxonomy lists, and with Views as part of core rather than as contrib it becomes even less likely - so why not just punt that entire chunk of functionality, with all associated settings, over to Views? Views does it better anyway.

klonos’s picture

I agree that now with views in core this issue needs to be revisited and rethought.

Anyways, I'd like to comment on what @jenlampton said back in #49:

...We have two different options, either the front page is /node, or it's not....

Well, now we have three options the way I see it:

- front page is a view (could be a listing of a specific content type once we have #1210366: Per-bundle node listing pages, blocks, feeds.)
- front page is a single static piece of content, a specific node.
- a "special" page like the "Welcome to your site" page.

I have given this some thought (after reading through related issues like #545758: Make q=node behavior optional/configurable, #404300: "Promote to front page" is a misnomer, #987242: The "Promoted to front page" checkbox doesn't do anything if the /node front page listing isn't used and other related issues) and would like to propose something, but I'm not sure I should do that here or open a new issue. It involves adding a radio button set like @jenlampton proposed back in #49 but with options being selecting a view from a drop-down or existing content through an autocomplete widget.

klonos’s picture

For one, I want to see #1201580: Move Front page & Error page settings out of "Site information" and into their own respective config pages. fixed first. Then, I would like to see @jenlampton's mockup from #49 become something like this:

Front page:
(*) A listing of content: [ Latest content    | v ]   configure...
( ) A specific piece of content: [ Welcome to our homepage     o ]    edit...
( ) A custom path: http://our.site.net/ [ my/custom/path         ]

In the above mockup the first option is a drop-down menu that lists all enabled views and has a 'configure' link that takes people to the selected view in edit mode. Once we have #1210366: Per-bundle node listing pages, blocks, feeds., the drop-down will include options like 'Articles listing', 'Basic pages listing', 'Users listing' as well as any custom view. The 'Latest content' view is actually #2029199: Rename the 'Frontpage' view to 'Latest content' or 'Featured content'. and will be the default out-of-the box setting.

The second option is an autocomplete field that looks up all content and has a 'edit' link that takes people to the selected piece of content in edit mode.

The third option is what we currently have in place now and it is there for allowing people to set things that are not available in either the first or second option as their home page.

Of course advanced and long-time users of Drupal know that they can achieve both the first and the second options with the custom path. This whole thing is simply a helper UI with shortcuts for novice users.

justinchev’s picture

Issue summary: View changes

See this thread for the patch which places the 'Frontpage' references with 'Promoted' instead. This includes renaming the 'Frontpage' view.
https://www.drupal.org/node/2029199

swentel’s picture

Status: Needs work » Closed (won't fix)

This is being removed in #2401035: items_per_page in node.settings is no longer used (since frontpage and taxonomy are now views)