WordPress.org

Make WordPress Core

Opened 7 months ago

Last modified 6 months ago

#42321 assigned defect (bug)

Menu title field does not clear invalid class

Reported by: ritukaushal5693 Owned by: welcher
Milestone: 5.0 Priority: normal
Severity: normal Version: 4.8.2
Component: Menus Keywords: good-first-bug has-patch early commit
Focuses: ui, administration Cc:

Description (last modified by welcher)

When we create the new menu and do not give any menu name and click on create a menu button than validation is shown in Menu name option. When we Fill the name of the menu validation color not changed. It’s still showing in red color. And Validation link is not showing in second create menu button.

Attachments (11)

screenhot-1.png (43.1 KB) - added by ritukaushal5693 7 months ago.
good-first-bug
screenhot-2.png (41.7 KB) - added by ritukaushal5693 7 months ago.
nav-menu.js.patch (752 bytes) - added by ashokrd2013 7 months ago.
nav-menu.min.js.patch (41.7 KB) - added by piyush9100 7 months ago.
I have changed in nav-menu.min.js to overcome this issues.
42321.patch (1.2 KB) - added by ashokrd2013 7 months ago.
@welcher I have updated my patch as per your guideline.
42321.1.patch (1.7 KB) - added by ashokrd2013 7 months ago.
@welcher updated my patch
42321.3.patch (1.7 KB) - added by welcher 7 months ago.
Refreshed patch
42321.4.patch (2.3 KB) - added by ashokrd2013 7 months ago.
Removed red outline from button
42321.2.patch (2.9 KB) - added by welcher 7 months ago.
Adding new css selector for invalid inputs
42321.5.patch (2.9 KB) - added by welcher 7 months ago.
Update patch to remove jshint error
42321.6.patch (2.8 KB) - added by welcher 6 months ago.
Patch refresh post standards update

Download all attachments as: .zip

Change History (25)

@ritukaushal5693
7 months ago

good-first-bug

#1 @SergeyBiryukov
7 months ago

  • Component changed from General to Menus
  • Focuses ui added

#2 @SergeyBiryukov
7 months ago

  • Focuses administration added

#3 @welcher
7 months ago

  • Keywords needs-patch good-first-bug added

Thanks for reporting!

Based on how some other forms are handled in core ( ie. New User ) we should be clearing the red border onBlur which is not the case here.

#4 @ashokrd2013
7 months ago

  • Keywords has-patch added; needs-patch removed

@welcher

I have patch for this. Please check it.

#5 @welcher
7 months ago

@ashokrd2013 thanks for the patch!

Just a couple feedback items for you:

  1. Please generate the patch from the root of the entire repo - the same level as the /src and /tests folders. This allows the patch be applied very easily and if there is a need for unit tests, they can be in the same diff.
  1. Can we use _.debounce on the keyup event to reduce the number of times the callback gets run?
  1. There are some coding standard issues in the patch. Have a read of https://make.wordpress.org/core/handbook/best-practices/coding-standards/javascript/#spacing .
  1. It's a small thing but menuName = $( document.getElementById( 'menu-name' ) ) is more performant than menuName = $( '#menu-name' )- https://jsperf.com/wrap-an-element-or-html-collection-in-jquery

Does this patch also address the issue with the missing border on the Create Menu button as well? If not, no worries we can iterate on it.

Thanks for working on this!

Last edited 7 months ago by welcher (previous) (diff)

@piyush9100
7 months ago

I have changed in nav-menu.min.js to overcome this issues.

#6 @harshbarach
7 months ago

Hey @piyush9100 , I have checked your patch and it's working awesome. Greatly appreciated.

@ashokrd2013
7 months ago

@welcher I have updated my patch as per your guideline.

#7 @welcher
7 months ago

  • Keywords needs-patch added; has-patch removed

@ashokrd2013 thanks for the update.

My only suggestion would be to make underscore a dependency when nav-menu is registered inscript-loader.php rather than calling wp_enqueue_script in nav-menus.php - https://github.com/WordPress/wordpress-develop/blob/master/src/wp-includes/script-loader.php#L878

Beyond the change above, there are still some best practices issues in regards to whitepace for example !menuNameVal should be ! menuNameVal etc. Have a read here for more details - https://make.wordpress.org/core/handbook/best-practices/coding-standards/javascript/#spacing.

#8 @welcher
7 months ago

@piyush9100 thanks for the patch!

However, the .min files are created part of the build process. Can you move your changes to the /src file? Perhaps take a look at the patch that @ashokrd2013 has put together and see if you can add to it.

@ashokrd2013
7 months ago

@welcher updated my patch

#9 @welcher
7 months ago

  • Description modified (diff)
  • Owner set to ashokrd2013
  • Status changed from new to assigned
  • Summary changed from Issue in create a new menu to Menu title field does not clear invalid class

@ashokrd2013 I've added some change to your patch to ensure that we're defining the JS vars before using them and to add some more spacing for readability. Great work!

Originally, this ticket also called out only one button having the red outline after the submit. I don't think it should be there at all. It seems like awkward UX and looks horrible ( to my color blind eyes anyway ). As such, I think we should stop it from being added to the button at all.

This is not going to make it before 4.9 RC ( the deadline for bugs is today ) so let's try to get that added to this patch and we'll aim for the next release.

I'm going to assign this to @ashokrd2013 but if anyone wants to work on it ( looking at you @piyush9100 :) ) please feel free to!

@welcher
7 months ago

Refreshed patch

#10 @welcher
7 months ago

@ashokrd2013 my last patch didn't have the latest code in it from master. I've refreshed it.

@ashokrd2013
7 months ago

Removed red outline from button

#11 @welcher
7 months ago

  • Keywords has-patch needs-testing added; needs-patch removed

@welcher
7 months ago

Adding new css selector for invalid inputs

#12 @welcher
7 months ago

  • Keywords needs-testing removed
  • Milestone changed from Awaiting Review to 4.9.1

I've done a slight refactor on the approach here.

Now instead of adding the form-invalid class to the div that wraps the entire form, I've added a new input.invalid selector which is added/removed to the input directly. This should prevent any unintended side effects of changing CSS.

Also, I have added an inline comment in scripts-loader.php that references this ticket to explain why we now have underscore as a dependency for nav-menu.js as that is not consistent across scripts loaded in wp-admin.

I think this is ready to go for 4.9.1

@welcher
7 months ago

Update patch to remove jshint error

#13 @johnbillion
6 months ago

  • Milestone changed from 4.9.1 to 5.0

#14 @welcher
6 months ago

  • Keywords early commit added
  • Owner changed from ashokrd2013 to welcher

@welcher
6 months ago

Patch refresh post standards update

Note: See TracTickets for help on using tickets.