Make WordPress Core

Opened 12 years ago

Closed 7 years ago

Last modified 7 years ago

#21492 closed defect (bug) (fixed)

Theme customizer > Static front page: missing error message when front page and posts pages are similar

Reported by: hd-j's profile hd-J Owned by: westonruter's profile westonruter
Milestone: 4.9 Priority: normal
Severity: normal Version:
Component: Customize Keywords: has-patch has-screenshots
Focuses: ui Cc:

Description

Steps to reproduce:

  1. Activate Twenty Eleven
  2. Open the Customizer
  3. In "Static Front page", choose the same page as your front page and Posts page:

Would it be possible to display the same warning in the Customizer, to avoid any confusion for the users?

Related: #19627 and #16379

Attachments (9)

21492.patch (1.6 KB) - added by ocean90 11 years ago.
index.png (337.0 KB) - added by MatheusGimenez 8 years ago.
Screenshot
21492.diff (3.8 KB) - added by MatheusGimenez 8 years ago.
21492.2.diff (3.5 KB) - added by MatheusGimenez 8 years ago.
21492.3.diff (3.2 KB) - added by MatheusGimenez 8 years ago.
change (int) to absint() to convert values
21492.4.diff (1.3 KB) - added by MatheusGimenez 8 years ago.
21492.5.diff (1.4 KB) - added by MatheusGimenez 8 years ago.
add phpdoc comment
21492.6.diff (5.3 KB) - added by MatheusGimenez 8 years ago.
error-notification.png (170.5 KB) - added by westonruter 7 years ago.
Notification added via Customizer notifications API

Download all attachments as: .zip

Change History (49)

#1 @SergeyBiryukov
12 years ago

  • Component changed from Themes to Appearance

#2 @jkudish
12 years ago

Last edited 12 years ago by jkudish (previous) (diff)

#3 @jkudish
12 years ago

  • Cc joachim.kudish@… added

This ticket was mentioned in IRC in #wordpress-dev by ericandrewlewis. View the logs.


11 years ago

@ocean90
11 years ago

#5 follow-up: @ocean90
11 years ago

  • Focuses ui added
  • Keywords has-patch added
  • Milestone changed from Awaiting Review to Future Release

Interesting that we have this warning, which seems a bit useless.

21492.patch is a first try to bring the warning to the Customizer. Currently it only compares the values when a setting is changed and shows the same message if the values are equal. Another idea would be to just remove the value from the other menu.

Also I'm not sure if should just wait for #19627 and #16379.

#7 @chriscct7
9 years ago

  • Keywords dev-feedback added

#8 in reply to: ↑ 5 @westonruter
9 years ago

Replying to ocean90:

Another idea would be to just remove the value from the other menu.

If we did that, then we should also display a notice ensuring that the user sees that the other setting has been reset, and explain that the two page designations can't point to the same page.

#9 @lancewillett
9 years ago

This came up recently in Calypso GitHub: https://github.com/Automattic/wp-calypso/issues/3085 — similar discussion. I think it'd be a nice enhancement, especially for new users — to avoid the confusion of setting the same page for both Posts and Static Home Page.

#10 @sixhours
8 years ago

Would love to see this happen. It's a common source of user confusion. I started a patch, but it's written in jQuery; a vanilla JS solution like the above would probably be better.

This ticket was mentioned in Slack in #core-customize by celloexpressions. View the logs.


8 years ago

#12 @westonruter
8 years ago

  • Keywords needs-patch added; has-patch dev-feedback removed

This should be re-worked to use the new setting validation API. If page_on_front is set to the same as page_for_posts it can fail validation.

Furthermore, this can be improved further to prevent the two dropdowns from selecting the same page to begin with. Once a selection is made in the first dropdown, the corresponding option in the second dropdown can be disabled to prevent it from even being selected. I recently implemented this on a Select2 replacement for the existing dropdown-pages controls: https://github.com/xwp/wp-customize-object-selector/pull/22

#13 @sixhours
8 years ago

This is the solution we're using on WordPress.com, in hopes it's helpful: https://github.com/sixhours/wp-customizer-fix

This ticket was mentioned in Slack in #core-customize by melchoyce. View the logs.


8 years ago

#15 @westonruter
8 years ago

  • Milestone changed from Future Release to 4.7.3

This ticket was mentioned in Slack in #core by jeffpaul. View the logs.


8 years ago

@MatheusGimenez
8 years ago

Screenshot

@MatheusGimenez
8 years ago

#17 @MatheusGimenez
8 years ago

  • Keywords has-patch added; needs-patch removed

Hi everybody.

I solved using the validation api. Check this out on the screenshot.

@MatheusGimenez
8 years ago

change (int) to absint() to convert values

#18 @westonruter
8 years ago

@MatheusGimenez it looks like your patch has additional unrelated media changes?

Also, is it handling the case where the user changes both the page_for_posts and the page_on_front in the same session? To handle this case, you should make sure you get the dirty value if it exists for the setting you want to check compare with. For example:

  $page_for_posts = absint( get_option( 'page_for_posts', 0 ) ); 
+ $page_for_posts = $this->get_setting( 'page_for_posts' )->post_value( $page_for_posts ); 

When you pass a value into the post_value method then it is used as the default if there was no dirty post value (in the current changeset) or it is invalid.

#19 @westonruter
8 years ago

@MatheusGimenez also, I think that absint shouldn't be used, per #38203. So maybe this to ensure it is not a negative value:

$page_for_posts = max( 0, (int) get_option( 'page_for_posts', 0 ) );

Additionally, instead of saying “warning” and “advise” the error message would need to be more forceful because saving would actually be blocked if they make the two the same.

#20 @MatheusGimenez
8 years ago

@westonruter I'm sorry, by mistake I left some stuff from another patch I worked with, I forgot to revert it before initiating the work on this one, I'm really sorry.

I've done another patch, changing the logic also and I've used @sixhours code as basis to work accordingly with the JS validation API.

Check this out (video): https://cloudup.com/cQRb_VzN1qZ

@MatheusGimenez
8 years ago

add phpdoc comment

#21 follow-up: @westonruter
8 years ago

  • Keywords ux-feedback added

@MatheusGimenez it looks like you didn't include customizer-static-front.js in your patch. Hard to tell exactly what the behavior is from the video, but it looks like a user is able to momentarily select the same pages and then after a short delay the duplicated setting is cleared out and the option is made disabled so that it cannot be re-selected? This causes an extra refresh and it may make the user confused as to why the error message just went away on its own.

If we want to still allow a user to select an identical page for posts as the page on front (for parity with admin screen), we can still allow this and make use of the JS notifications API by adding a warning notification instead of an error notification. See example. As such there wouldn't be any server-side component at all. I know I suggested above the whole disabling of the option and to use server-side validation to block the user from being able to save, but perhaps in the end a modern iteration on @ocean90's original 21492.patch would be best, if it was updated to use the new JS notifications API.

@karmatosed thoughts on the UX? Should we still allow the user to select the same page for posts ad page on front, even when we warn them not to?

#22 follow-up: @karmatosed
8 years ago

I think we shouldn't allow them to select it if we warn them. I'm trying to think of a use case where it would be something you'd want - I can't think of one. If we can't find one, then it should not allow selection.

Looking at the video, we probably want to iterate on the message a little. Perhaps 'Your posts and front page should not be the same'. I also think it's hard to see what is going on. I'm concerned about timing of the message.

Last edited 8 years ago by karmatosed (previous) (diff)

#23 in reply to: ↑ 21 @MatheusGimenez
8 years ago

Replying to westonruter:

@MatheusGimenez it looks like you didn't include customizer-static-front.js in your patch.

Sorry. I'm not SVN guy, i guess, hehe.

Hard to tell exactly what the behavior is from the video, but it looks like a user is able to momentarily select the same pages and then after a short delay the duplicated setting is cleared out and the option is made disabled so that it cannot be re-selected? This causes an extra refresh and it may make the user confused as to why the error message just went away on its own.

If we want to still allow a user to select an identical page for posts as the page on front (for parity with admin screen), we can still allow this and make use of the JS notifications API by adding a warning notification instead of an error notification. See example. As such there wouldn't be any server-side component at all. I know I suggested above the whole disabling of the option and to use server-side validation to block the user from being able to save, but perhaps in the end a modern iteration on @ocean90's original 21492.patch would be best, if it was updated to use the new JS notifications API.

@karmatosed thoughts on the UX? Should we still allow the user to select the same page for posts ad page on front, even when we warn them not to?

I honestly do not know why the message is automatically disappearing after a few seconds. First I thought it was some feature of the validation api, but with your comment it seems it is not.

Even if I remove the function that removes the selected option, the notification continues to disappear.

Watch on video:

https://cloudup.com/crL07SjjxHt

#24 @westonruter
8 years ago

  • Milestone changed from 4.7.3 to 4.7.4

This ticket was mentioned in Slack in #core by swissspidy. View the logs.


8 years ago

This ticket was mentioned in Slack in #core by swissspidy. View the logs.


8 years ago

This ticket was mentioned in Slack in #core by swissspidy. View the logs.


8 years ago

#28 @swissspidy
8 years ago

  • Milestone changed from 4.7.4 to 4.8

Moving to the 4.8 milestone as discussed during today's bug scrub

#29 in reply to: ↑ 22 @melchoyce
8 years ago

Replying to karmatosed:

Looking at the video, we probably want to iterate on the message a little. Perhaps 'Your posts and front page should not be the same'. I also think it's hard to see what is going on. I'm concerned about timing of the message.

+1, or something like @sixhours' version:

Front page & Posts page must be different.

@michelleweber and @zoonini, do you have and preference re: wording?

#30 follow-up: @zoonini
8 years ago

The wording on WordPress.com that we put into place with @sixhours' patch has proved to be quite clear for users, so I'm in favour of keeping it, or something close to it.

Front page & Posts page must be different.

Here's what it looks like in action:

https://cldup.com/7L4GbDXrfy.png

#31 in reply to: ↑ 30 @michelleweber
8 years ago

If this is what we're using and it's clear to folks, I'd +1 going with this.

Replying to zoonini:

The wording on WordPress.com that we put into place with @sixhours' patch has proved to be quite clear for users, so I'm in favour of keeping it, or something close to it.

Front page & Posts page must be different.

Here's what it looks like in action:

https://cldup.com/7L4GbDXrfy.png

This ticket was mentioned in Slack in #core by obenland. View the logs.


7 years ago

#33 @obenland
7 years ago

  • Milestone changed from 4.8 to Future Release

#34 @westonruter
7 years ago

  • Milestone changed from Future Release to 4.8.1

I believe improving the behavior of setting the static front page is going to be priority for the next release.

#35 @westonruter
7 years ago

  • Milestone changed from 4.8.1 to 4.9

This ticket was mentioned in Slack in #core-customize by jeffpaul. View the logs.


7 years ago

#37 @westonruter
7 years ago

  • Owner set to westonruter
  • Status changed from new to accepted

@westonruter
7 years ago

Notification added via Customizer notifications API

#38 @westonruter
7 years ago

  • Keywords has-screenshots added; ux-feedback removed

#39 @westonruter
7 years ago

  • Resolution set to fixed
  • Status changed from accepted to closed

In 41389:

Customize: Show notification error with "Your homepage displays" control when homepage and posts page are set to be the same (but not empty).

  • Show global error notiafication when saving is blocked due to client-side setting invalidity.
  • Refactor wp.customize.Notifications#render() to ensure a notification re-renders if its message or data changes but its code does not.

Props MatheusGimenez, sixhours, westonruter, karmatosed, aocean90, zoonini, michelleweber, melchoyce.
See #35210.
Fixes #21492.

#40 @ocean90
7 years ago

In 41791:

Customize: Use a consistent translator comment for the same strings.

See #21492.

Note: See TracTickets for help on using tickets.