Added keyboard navigation for Adobe Stock image previews #25611

drpayyne · 2019-11-14T19:12:22Z

Description

Added left//right arrow navigation for image preview in grid
Various checks involved to prevent JS errors
- Check if image preview is opened
- Check if Adobe Stock panel is opened
- Prevent left navigation for first image
- Prevent right navigation for last image

Fixed Issues

Cannot navigate to next image in preview using keyboard arrows adobe-stock-integration#691: Cannot navigate to next image in preview using keyboard arrows

Manual testing scenarios

Open Adobe Stock panel
Open any image preview
Navigate left and right with keyboard arrow keys

m2-assistant · 2019-11-14T19:12:26Z

Hi @drpayyne. Thank you for your contribution
Here is some useful tips how you can test your changes using Magento test environment.
Add the comment under your pull request to deploy test or vanilla Magento instance:

@magento give me test instance - deploy test instance based on PR changes
@magento give me 2.3-develop instance - deploy vanilla Magento instance

For more details, please, review the Magento Contributor Guide documentation.

sivaschenko

Hi @drpayyne thank you for the pull request, please check my review comments

sivaschenko · 2019-11-18T11:51:53Z

app/code/Magento/Ui/view/base/web/js/grid/columns/image-preview.js

+         * Set image preview keyboard navigation listener
+         */
+        setNavigationListener: function () {
+            var imageIndex, endIndex, key,


I don't think start and endIndex should be handled here. They should be handled in next and prev methods (if it is currently possible to trigger them for first/last image)

Fixed this too, thank you for your guidance!

sivaschenko · 2019-11-18T11:54:23Z

app/code/Magento/Ui/view/base/web/js/grid/columns/image-preview.js

+                    if(key === 'pageLeftKey' && imageIndex !== startIndex) {
+                        $(this.previewImageSelector + ' .action-previous').click();
+                    } else if (key === 'pageRightKey' && imageIndex !== endIndex) {
+                        $(this.previewImageSelector + ' .action-next').click();


Shouldn't next/prev methods of this component be called directly instead of triggering a click?

Didn't realize displayedRecord property can be used for that purpose. Fixed it now, thank you!

sivaschenko · 2019-11-20T13:57:19Z

app/code/Magento/Ui/view/base/web/js/grid/columns/image-preview.js

+            $(document).on('keydown', function (e) {
+                key = keyCodes[e.keyCode];
+
+                if ($(adobeModalSelector).hasClass('_show') && $(self.previewImageSelector).length > 0) {


It's better to unbind this check from visual representation as well

Suggested change

if ($(adobeModalSelector).hasClass('_show') && $(self.previewImageSelector).length > 0) {

if (this.visibleRecord() !== null) {

Fixed this, thanks!

sivaschenko · 2019-11-20T14:13:53Z

app/code/Magento/Ui/view/base/web/js/grid/columns/image-preview.js

+         */
+        setNavigationListener: function () {
+            var key,
+                self = this,


Please bind this to function instead of using self variable

sivaschenko · 2019-11-20T14:14:08Z

app/code/Magento/Ui/view/base/web/js/grid/columns/image-preview.js

+        setNavigationListener: function () {
+            var key,
+                self = this,
+                adobeModalSelector = '.adobe-stock-modal';


This variable is not used

drpayyne · 2019-11-20T15:46:58Z

Hi @sivaschenko, I've done your suggested changes, refactored the code to optimize it a bit, and tested too. Thanks!

sivaschenko

Thanks for updates! One more issue arose after the reorganization of the code:

sivaschenko · 2019-11-20T15:54:53Z

app/code/Magento/Ui/view/base/web/js/grid/columns/image-preview.js

+        /**
+         * Set image preview keyboard navigation listener
+         */
+        setNavigationListener: function (e) {


After the reorganization, the better name for the function would be 'handleKeydown'. Please add the method params/return to the docblock and update the comment to be up to date with the actual method purpose

Suggested change

setNavigationListener: function (e) {

handleKeydown: function (event) {

Fixed it, thanks!

magento-engcom-team · 2019-11-25T17:56:42Z

Hi @sivaschenko, thank you for the review.
ENGCOM-6331 has been created to process this Pull Request
✳️ @sivaschenko, could you please add one of the following labels to the Pull Request?

Label	Description
`Auto-Tests: Covered`	All changes in Pull Request is covered by auto-tests
`Auto-Tests: Not Covered`	Changes in Pull Request requires coverage by auto-tests
`Auto-Tests: Not Required`	Changes in Pull Request does not require coverage by auto-tests

engcom-Delta · 2019-11-26T07:44:32Z

@magento give me test instance

magento-engcom-team · 2019-11-26T07:44:38Z

Hi @engcom-Delta. Thank you for your request. I'm working on Magento instance for you

magento-engcom-team · 2019-11-26T07:45:17Z

Hi @engcom-Delta, here is your new Magento instance.
Admin access: https://pr-25611.instances.magento-community.engineering/admin
Login: admin Password: 123123q

engcom-Delta · 2019-11-26T07:46:00Z

@magento give me 2.3-develop instance

magento-engcom-team · 2019-11-26T07:46:03Z

Hi @engcom-Delta. Thank you for your request. I'm working on Magento 2.3-develop instance for you

magento-engcom-team · 2019-11-26T07:46:45Z

Hi @engcom-Delta, here is your Magento instance.
Admin access: https://i-25611-2-3-develop.instances.magento-community.engineering/admin
Login: admin Password: 123123q
Instance will be terminated in up to 3 hours.

engcom-Delta · 2019-11-26T08:06:58Z

✔️ QA passed

magento-engcom-team · 2019-11-26T13:35:43Z

Hi @sivaschenko, thank you for the review.
ENGCOM-6331 has been created to process this Pull Request
✳️ @sivaschenko, could you please add one of the following labels to the Pull Request?

Label	Description
`Auto-Tests: Covered`	All changes in Pull Request is covered by auto-tests
`Auto-Tests: Not Covered`	Changes in Pull Request requires coverage by auto-tests
`Auto-Tests: Not Required`	Changes in Pull Request does not require coverage by auto-tests

sivaschenko · 2019-11-26T13:40:46Z

Covered by functional tests in Adobe Stock Integration

…25611

m2-assistant · 2019-11-27T16:46:56Z

Hi @drpayyne, thank you for your contribution!
Please, complete Contribution Survey, it will take less than a minute.
Your feedback will help us to improve contribution process.

Resolved Adobe issue 691

0e30366

drpayyne requested a review from sivaschenko November 14, 2019 19:12

drpayyne requested a review from DrewML as a code owner November 14, 2019 19:12

magento-engcom-team added Area: Frontend Component: Ui Release Line: 2.3 Partner: EY partners-contribution Pull Request is created by Magento Partner labels Nov 14, 2019

sivaschenko requested review from VladimirZaets and omiroshnichenko November 14, 2019 20:07

sivaschenko requested changes Nov 18, 2019

View reviewed changes

ghost assigned sivaschenko Nov 18, 2019

ghost added the Progress: needs update label Nov 18, 2019

drpayyne force-pushed the adobe-issue-691 branch from 9e04804 to 05285c2 Compare November 19, 2019 15:50

drpayyne requested a review from sivaschenko November 19, 2019 15:50

sivaschenko requested changes Nov 20, 2019

View reviewed changes

drpayyne force-pushed the adobe-issue-691 branch from 05285c2 to 5e45173 Compare November 20, 2019 14:08

drpayyne requested a review from sivaschenko November 20, 2019 14:08

sivaschenko requested changes Nov 20, 2019

View reviewed changes

drpayyne force-pushed the adobe-issue-691 branch from 5e45173 to 0afb5d3 Compare November 20, 2019 15:45

drpayyne requested a review from sivaschenko November 20, 2019 15:47

sivaschenko requested changes Nov 20, 2019

View reviewed changes

drpayyne force-pushed the adobe-issue-691 branch 2 times, most recently from f173c8d to d6d4cc6 Compare November 20, 2019 16:08

drpayyne requested a review from sivaschenko November 20, 2019 16:09

sivaschenko approved these changes Nov 25, 2019

View reviewed changes

ghost added Progress: ready for testing and removed Progress: needs update labels Nov 25, 2019

Refactored action click

1298766

drpayyne force-pushed the adobe-issue-691 branch from d6d4cc6 to 1298766 Compare November 26, 2019 05:29

engcom-Delta self-assigned this Nov 26, 2019

ghost added Progress: testing in progress and removed Progress: ready for testing labels Nov 26, 2019

ghost added Progress: extended testing and removed Progress: testing in progress labels Nov 26, 2019

magento#25611: Static test fix.

41c95a2

ghost added Progress: accept and removed Progress: extended testing labels Nov 26, 2019

sivaschenko approved these changes Nov 26, 2019

View reviewed changes

sivaschenko added the Auto-Tests: Covered All changes in Pull Request is covered by auto-tests label Nov 26, 2019

magento-engcom-team mentioned this pull request Nov 27, 2019

Readonly inputs and after element html in the backend #691

Closed

magento-engcom-team pushed a commit that referenced this pull request Nov 27, 2019

ENGCOM-6331: Added keyboard navigation for Adobe Stock image previews #…

83d9d7c

…25611

magento-engcom-team merged commit 41c95a2 into magento:2.3-develop Nov 27, 2019

magento-engcom-team added this to the Release: 2.3.5 milestone Nov 27, 2019

drpayyne mentioned this pull request Dec 1, 2019

Cannot navigate to next image in preview using keyboard arrows magento/adobe-stock-integration#691

Closed

sivaschenko added the Award: bug fix label Dec 12, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added keyboard navigation for Adobe Stock image previews #25611

Added keyboard navigation for Adobe Stock image previews #25611

drpayyne commented Nov 14, 2019

m2-assistant bot commented Nov 14, 2019

sivaschenko left a comment

sivaschenko Nov 18, 2019

drpayyne Nov 19, 2019

sivaschenko Nov 18, 2019

drpayyne Nov 19, 2019

sivaschenko Nov 20, 2019

drpayyne Nov 20, 2019

sivaschenko Nov 20, 2019

sivaschenko Nov 20, 2019

drpayyne commented Nov 20, 2019

sivaschenko left a comment

sivaschenko Nov 20, 2019

drpayyne Nov 20, 2019

magento-engcom-team commented Nov 25, 2019

engcom-Delta commented Nov 26, 2019

magento-engcom-team commented Nov 26, 2019

magento-engcom-team commented Nov 26, 2019

engcom-Delta commented Nov 26, 2019

magento-engcom-team commented Nov 26, 2019

magento-engcom-team commented Nov 26, 2019

engcom-Delta commented Nov 26, 2019

magento-engcom-team commented Nov 26, 2019

sivaschenko commented Nov 26, 2019

m2-assistant bot commented Nov 27, 2019

	if ($(adobeModalSelector).hasClass('_show') && $(self.previewImageSelector).length > 0) {
	if (this.visibleRecord() !== null) {

	setNavigationListener: function (e) {
	handleKeydown: function (event) {

Added keyboard navigation for Adobe Stock image previews #25611

Added keyboard navigation for Adobe Stock image previews #25611

Conversation

drpayyne commented Nov 14, 2019

Description

Fixed Issues

Manual testing scenarios

m2-assistant bot commented Nov 14, 2019

sivaschenko left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

drpayyne commented Nov 20, 2019

sivaschenko left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

magento-engcom-team commented Nov 25, 2019

engcom-Delta commented Nov 26, 2019

magento-engcom-team commented Nov 26, 2019

magento-engcom-team commented Nov 26, 2019

engcom-Delta commented Nov 26, 2019

magento-engcom-team commented Nov 26, 2019

magento-engcom-team commented Nov 26, 2019

engcom-Delta commented Nov 26, 2019

magento-engcom-team commented Nov 26, 2019

sivaschenko commented Nov 26, 2019

m2-assistant bot commented Nov 27, 2019