-
Notifications
You must be signed in to change notification settings - Fork 9.3k
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.
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
Fixed issue when the preview images navigation is triggered by moving the input filed cursor using arrow keys #25991
Fixed issue when the preview images navigation is triggered by moving the input filed cursor using arrow keys #25991
Conversation
Hi @drpayyne. Thank you for your contribution
For more details, please, review the Magento Contributor Guide documentation. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR @drpayyne , please take a look at the comment below.
It will be great if you could also add a test coverage for this case as part of the PR: either MFTF or js
@@ -46,7 +46,7 @@ define([ | |||
*/ | |||
initialize: function () { | |||
this._super(); | |||
$(document).on('keydown', this.handleKeyDown.bind(this)); | |||
$('.modal-slide.adobe-stock-modal').on('keydown', this.handleKeyDown.bind(this)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you please move this selector to defaults
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done, thanks! I also added a check to see if an input element is active to prevent image navigation when the search textbox is active. This also works for the image quantity for the page, but for some reason, not for the page number counter itself... Any thoughts?
Related: magento/adobe-stock-integration#846
54982c2
to
2c3d5dd
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for update @drpayyne ! Please see my comment
@@ -13,6 +13,7 @@ define([ | |||
defaults: { | |||
bodyTmpl: 'ui/grid/columns/image-preview', | |||
previewImageSelector: '[data-image-preview]', | |||
adobeStockModalSelector: '.modal-slide.adobe-stock-modal', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can the selector (and property) be modified not to mention adobe stock (as that is not currently adobe stock scope)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed this too, thanks!
2c3d5dd
to
9f1be1d
Compare
@sivaschenko, ICYMI. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the pull request @drpayyne ! Please see my comment
@@ -130,6 +131,11 @@ define([ | |||
show: function (record) { | |||
var img; | |||
|
|||
if (!this.isListenerActive) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe we can check for visibleRecord
in handleKeyDown
without introducing the new field and show method logic complication
Hi @drpayyne, thank you for your contribution! |
Sorry, closed the wrong pull request by mistake |
I will take care of test coverage. |
Hi @sivaschenko could you please review test coverage ? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for test coverage @Nazar65 ! Please see my review comments
it('veify record changed on key down', function () { | ||
var recordMock = { | ||
_rowIndex: 2 | ||
}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Nazar65 @engcom-Golf the formatting is not consistent, lines 53-54 require additional indentation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok
@@ -46,5 +46,39 @@ define([ | |||
}); | |||
|
|||
}); | |||
|
|||
describe('handleKeyDown method', function () { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be good also to test the case when the active element is input
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok
Hi @sivaschenko, thank you for the review. |
…red by moving the input filed cursor using arrow keys #25991
Hi @drpayyne, thank you for your contribution! |
Description
Fixed Issues
Manual testing scenarios
Contribution checklist