Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fixing #10275 keyboard submit of adminhtml suggest form. #11250

Merged
merged 3 commits into from
Oct 26, 2017
Merged

Fixing #10275 keyboard submit of adminhtml suggest form. #11250

merged 3 commits into from
Oct 26, 2017

Conversation

romainruaud
Copy link
Contributor

@romainruaud romainruaud commented Oct 5, 2017 •
edited by ishakhsuvarov
Loading

This is a fix for #10275

Description

Pressing ENTER when navigating through admin suggest now work as expected.

Fixed Issues (if relevant)

  1. Admin global search - submit by enter doesn't work #10275 : Admin global search - submit by enter doesn't work

Manual testing scenarios

  1. Go to Magento 2 admin panel
  2. Click "Search" icon in header
  3. Put some query (as example 7029) in search field, wait for autocomplete
  4. Select "" in Products option with arrow keys on keyboard
  5. Click enter button
  6. You are redirected to the result page. Previously this was not working

Contribution checklist

  • Pull request has a meaningful description of its purpose
  • All commits are accompanied by meaningful commit messages
  • All new or changed code is covered with unit/integration tests (if applicable)
  • All automated tests passed successfully (all builds on Travis CI are green)

@romainruaud romainruaud force-pushed the fix_admin-search-menu branch 2 times, most recently from 6c84240 to 956fe08 Compare October 6, 2017 10:31
@ishakhsuvarov ishakhsuvarov self-assigned this Oct 6, 2017
@ishakhsuvarov ishakhsuvarov added this to the October 2017 milestone Oct 6, 2017
@romainruaud romainruaud force-pushed the fix_admin-search-menu branch from 956fe08 to eefdc1c Compare October 6, 2017 14:54
@romainruaud
Copy link
Contributor Author

Hmmm the test which failed seems to have nothing related with my PR :

1) Magento\Customer\Test\Unit\Model\AccountManagementTest::testInitiatePasswordResetEmailReminder

Expectation failed for method name is equal to <string:setRpTokenCreatedAt> when invoked zero or more times

Parameter 0 for invocation Magento\Customer\Model\Data\CustomerSecure::setRpTokenCreatedAt('2017-10-06 13:22:27') does not match expected value.

Failed asserting that two strings are equal.

What should I do now since I cannot restart the Travis build by myself @ishakhsuvarov ?

@mzeis
Copy link
Contributor

mzeis commented Oct 7, 2017

@romainruaud You can close and re-open the PR. This will trigger a new Travis build.

@romainruaud romainruaud closed this Oct 7, 2017
@romainruaud romainruaud reopened this Oct 7, 2017
@romainruaud
Copy link
Contributor Author

Travis timed out this time... I close/open to trigger again

@romainruaud romainruaud closed this Oct 8, 2017
@romainruaud romainruaud reopened this Oct 8, 2017
ishakhsuvarov
ishakhsuvarov requested changes Oct 10, 2017
View reviewed changes
Copy link
Contributor

@ishakhsuvarov ishakhsuvarov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please check the code review suggestions and consider covering this behavior update with the test

lib/web/mage/backend/suggest.js Outdated
@@ -245,6 +245,20 @@

case keyCode.ENTER:
case keyCode.NUMPAD_ENTER:
suggestList = event.currentTarget.parentNode.getElementsByTagName('ul')[0];
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there any specific reason not to use jQuery for this purpose?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ishakhsuvarov you are right, I could have used jQuery, but vanillaJS does good also for things like this, and the rest of the file is retrieving elements via the same syntax, so I remained consistent.

lib/web/mage/backend/suggest.js Outdated

if (hasSelectedItems) {
selectedItem = $(suggestList.getElementsByClassName('_active')[0]);
/* eslint-disable max-depth */
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We do not encourage suppressing warnings from linters. Please consider refactoring this part accordingly.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ishakhsuvarov Yep, I will move this part into a dedicated function I guess.

@ishakhsuvarov
Copy link
Contributor

Hi @romainruaud
Do you have any updates regarding this PR? Any help wanted?

@romainruaud
Copy link
Contributor Author

@ishakhsuvarov I was in holidays sorry.

I fix the changes ASAP

@romainruaud
Copy link
Contributor Author

@ishakhsuvarov done

@ishakhsuvarov
Copy link
Contributor

@romainruaud Thank you!

@ishakhsuvarov ishakhsuvarov added 2.2.x bug report Reproduced on 2.1.x The issue has been reproduced on latest 2.1 release Reproduced on 2.2.x The issue has been reproduced on latest 2.2 release labels Oct 23, 2017
@magento-team magento-team merged commit a0bef71 into magento:2.2-develop Oct 26, 2017
magento-team pushed a commit that referenced this pull request Oct 26, 2017
MAGETWO-81146: Fixing #10275 keyboard submit of adminhtml suggest form.
501b31a 
#11250
magento-team pushed a commit that referenced this pull request Oct 26, 2017
@ishakhsuvarov
Merge pull request #1625 from magento-engcom/2.2-develop-prs
04140ba 
[EngCom] Public Pull Requests - 2.2-develop
 - MAGETWO-82724 Allow coupon code with special charater to be applied to order in checkout #11710
 - MAGETWO-82675 Add a health check to the NGINX configuration sample #11690
 - MAGETWO-82562 Coupon codes not showing in invoice #11635
 - MAGETWO-82535 Fixed ability to set field config from layout xml #11302 [backport 2.2] #11643
 - MAGETWO-81146 Fixing #10275 keyboard submit of adminhtml suggest form. #11250
 - MAGETWO-82761 [Backport 2.2-develop] Dashboard Fix Y Axis for range #11751
 - MAGETWO-82748 Fix Notice: freePackageValue is undefined #11720
 - MAGETWO-82747 [TASK] Updated user.ini according to Magento DevDocs #11734
 - MAGETWO-82537 MAGETWO-81311: Check the length of the array before attempting to sli… #11637
 - MAGETWO-81970 Add missing translations in Magento_UI #11440
 - MAGETWO-81904 FIX #11022 in 2.2-develop: Filter Groups of search criteria parameter have not been included for further processing #11421
 - MAGETWO-82179 Fix Filter Customer Report Review 2.2-develop [Backport] #11522
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ishakhsuvarov ishakhsuvarov ishakhsuvarov approved these changes

Assignees

@ishakhsuvarov ishakhsuvarov

Labels
bug report Progress: needs update Release Line: 2.2 Reproduced on 2.1.x The issue has been reproduced on latest 2.1 release Reproduced on 2.2.x The issue has been reproduced on latest 2.2 release
Projects
None yet
Milestone
October 2017
Development

Successfully merging this pull request may close these issues.
5 participants
@romainruaud @mzeis @ishakhsuvarov @magento-team @vkublytskyi