WordPress.org

Make WordPress Core

Opened 2 years ago

Closed 9 months ago

#42321 closed defect (bug) (fixed)

Menu title field does not clear invalid class

Reported by: ritukaushal5693 Owned by: pento
Milestone: 5.1 Priority: normal
Severity: normal Version: 4.8.2
Component: Menus Keywords: good-first-bug has-patch commit
Focuses: ui, administration Cc:
PR Number:

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 (13)

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

Download all attachments as: .zip

Change History (35)

@ritukaushal5693
2 years ago

good-first-bug

#1 @SergeyBiryukov
2 years ago

  • Component changed from General to Menus
  • Focuses ui added

#2 @SergeyBiryukov
2 years ago

  • Focuses administration added

#3 @welcher
2 years 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
2 years ago

  • Keywords has-patch added; needs-patch removed

@welcher

I have patch for this. Please check it.

#5 @welcher
2 years 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 2 years ago by welcher (previous) (diff)

@piyush9100
2 years ago

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

#6 @harshbarach
2 years ago

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

@ashokrd2013
2 years ago

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

#7 @welcher
2 years 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
2 years 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
2 years ago

@welcher updated my patch

#9 @welcher
2 years 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
2 years ago

Refreshed patch

#10 @welcher
2 years ago

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

@ashokrd2013
2 years ago

Removed red outline from button

#11 @welcher
2 years ago

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

@welcher
2 years ago

Adding new css selector for invalid inputs

#12 @welcher
2 years 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
2 years ago

Update patch to remove jshint error

#13 @johnbillion
23 months ago

  • Milestone changed from 4.9.1 to 5.0

#14 @welcher
23 months ago

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

@welcher
23 months ago

Patch refresh post standards update

#15 @johnbillion
12 months ago

  • Milestone changed from 5.0 to 5.1

#16 @desrosj
9 months ago

  • Keywords needs-refresh added; commit removed

@welcher Are you able to refresh 42321.6.patch? It is no longer applying cleanly to trunk.

@welcher
9 months ago

Refreshed patch

#17 @welcher
9 months ago

  • Keywords needs-refresh removed

#18 @welcher
9 months ago

  • Keywords commit added; early removed

@desrosj patch refreshed.

#19 @afercia
9 months ago

  • Keywords needs-refresh added; commit removed

The patch should use the input event and not keyup:

  • keyup was generally used only to cover IE 8
  • input accounts also when pasting with the mouse
  • the menu name doesn't have a title attribute any longer so checking for menuName.attr('title') should be removed?
  • misses some coding standards (mainly: spaces)

Also, not sure the comment in script-loader.php should be a docblock starting with /**.

Last edited 9 months ago by afercia (previous) (diff)

@afercia
9 months ago

#20 @afercia
9 months ago

  • Keywords commit added; needs-refresh removed

42321.diff addresses the points above.
Also, I'd propose to keep using the form-invalid CSS class, as the new one on the single input doesn't account for the focus style: when invalid and focused, two box-shadows (red and blue) were applied to the field. We shouldn't change this part as it would require some unnecessary CSS refactoring. As far as I can tell, there are no concerns for unintended side effects as form-invalid is only applied to the menu name field parent.

#21 @pento
9 months ago

  • Owner changed from welcher to pento
  • Status changed from assigned to accepted

#22 @pento
9 months ago

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

In 44680:

Menus: Remove error styling when the user addresses the error.

An error is triggered if a Menu Name isn't given when the user attempts to create a menu. When they go back and add a name, the error styling can be removed.

Props ashokrd2013, piyush9100, welcher, afercia.
Fixes #42321.

Note: See TracTickets for help on using tickets.