WordPress.org

Make WordPress Core

Opened 5 years ago

Last modified 30 hours ago

#21492 new defect (bug)

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

Reported by: hd-J Owned by:
Milestone: 4.7.4 Priority: normal
Severity: normal Version:
Component: Customize Keywords: has-patch ux-feedback
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 (8)

21492.patch (1.6 KB) - added by ocean90 3 years ago.
index.png (337.0 KB) - added by MatheusGimenez 5 weeks ago.
Screenshot
21492.diff (3.8 KB) - added by MatheusGimenez 5 weeks ago.
21492.2.diff (3.5 KB) - added by MatheusGimenez 5 weeks ago.
21492.3.diff (3.2 KB) - added by MatheusGimenez 5 weeks ago.
change (int) to absint() to convert values
21492.4.diff (1.3 KB) - added by MatheusGimenez 5 weeks ago.
21492.5.diff (1.4 KB) - added by MatheusGimenez 5 weeks ago.
add phpdoc comment
21492.6.diff (5.3 KB) - added by MatheusGimenez 5 weeks ago.

Download all attachments as: .zip

Change History (34)

#1 @SergeyBiryukov
5 years ago

  • Component changed from Themes to Appearance

#2 @jkudish
5 years ago

related: #19627

Version 0, edited 5 years ago by jkudish (next)

#3 @jkudish
5 years ago

  • Cc joachim.kudish@… added

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


3 years ago

@ocean90
3 years ago

#5 follow-up: @ocean90
3 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
18 months ago

  • Keywords dev-feedback added

#8 in reply to: ↑ 5 @westonruter
18 months 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
11 months 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 months 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.


6 months ago

#12 @westonruter
6 months 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
5 months 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.


7 weeks ago

#15 @westonruter
7 weeks ago

  • Milestone changed from Future Release to 4.7.3

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


6 weeks ago

@MatheusGimenez
5 weeks ago

Screenshot

@MatheusGimenez
5 weeks ago

#17 @MatheusGimenez
5 weeks ago

  • Keywords has-patch added; needs-patch removed

Hi everybody.

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

@MatheusGimenez
5 weeks ago

change (int) to absint() to convert values

#18 @westonruter
5 weeks 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
5 weeks 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
5 weeks 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
5 weeks ago

add phpdoc comment

#21 follow-up: @westonruter
5 weeks 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 @karmatosed
5 weeks 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 5 weeks ago by karmatosed (previous) (diff)

#23 in reply to: ↑ 21 @MatheusGimenez
5 weeks 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
4 weeks 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 days ago

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


30 hours ago

Note: See TracTickets for help on using tickets.