Added masonry grid and columns components #25464

sivaschenko · 2019-11-04T15:20:01Z

Added masonry grid and columns components developed in the scope of Adobe Stock Integation project

Description (*)

Masonry grid and columns components allow creating images listings:

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 are green)

m2-assistant · 2019-11-04T15:20:04Z

Hi @sivaschenko. 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.

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

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

app/code/Magento/Ui/view/base/web/js/grid/columns/overlay.js

app/code/Magento/Ui/view/base/web/js/grid/masonry.js

omiroshnichenko · 2019-11-04T23:06:43Z

app/code/Magento/Ui/view/base/web/js/grid/masonry.js

+                        setTimeout(function () {
+                            handler();
+                            running = false;
+                        }, 66);


That is the optimal number for the timeout :) Any suggestions?

1000 / 60 to achieve 60fps ;)

^^you missed this comment

Might make more sense to just use the polyfill/fallback we have in place here. At least that way we're not inlining these fallbacks everytime we need to use rAF

I completely forgot that we have this.

@DrewML @omiroshnichenko can you please clarify what would you like to see here

omiroshnichenko · 2019-11-04T23:09:13Z

app/code/Magento/Ui/view/base/web/js/grid/masonry.js

+        /**
+         * Set layout styles when container element is loaded.
+         */
+        setLayoutStylesWhenLoaded: function () {


I don't see place where this function has been called. Should it be removed?

This function can be used when loading the masonry grid on a slide panel. For example: https://github.com/magento/adobe-stock-integration/blob/develop/AdobeStockImageAdminUi/view/adminhtml/web/panel.js

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

omiroshnichenko · 2019-11-04T23:12:17Z

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

+            <button class="action-next" type="button" data-bind="click: function () { $col.next($row()); }">
+                <span translate="'Next'"/>
+            </button>
+            <button class="action-close" type="button" data-bind="click: function () { $col.hide(); }">


Replace data-bind with click attribute

app/code/Magento/Ui/view/base/web/templates/grid/masonry.html

omiroshnichenko · 2019-11-04T23:14:52Z

app/code/Magento/Ui/view/base/web/js/grid/masonry.js

+         * Set min ratio for images in layout
+         */
+        setMinRatio: function () {
+            if (this.containerWidth <= 640) {


Better to move these values as a part of the component configuration in defaults

omiroshnichenko · 2019-11-04T23:15:21Z

app/code/Magento/Ui/view/base/web/js/grid/masonry.js

+        /**
+         * Wait for container to initialize
+         */
+        waitForContainer: function (callback) {


Related to comment below

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

sivaschenko · 2019-11-05T11:23:30Z

Hi @omiroshnichenko thank you for the review! I have applied all the required fixes and/or responded to the comments. Can you please very the code review changes

DrewML · 2019-11-05T20:06:26Z

app/code/Magento/Ui/view/base/web/js/grid/masonry.js

+         */
+        setLayoutStyles: function () {
+            var containerWidth = parseInt(this.container.clientWidth, 10),
+                rowImages = [],


The combination of an empty array above (rowImages) and a forEach below can be a good indication that map is better suited for the job. Would recommend switching over

I don't think map can be used for this algorithm, rowImages is a temporary array containing images that should be in a single visual row on the screen. This array is flushed and recreated several times (= number of images rows) during the foreach execution. this.rows().foreach iterating over each data row (each image in this case)

app/code/Magento/Ui/view/base/web/js/grid/masonry.js

DrewML · 2019-11-05T20:11:45Z

app/code/Magento/Ui/view/base/web/js/grid/masonry.js

+         * Set min ratio for images in layout
+         */
+        setMinRatio: function () {
+            var minRatio = null;


Seems like this placeholder var isn't needed, if we're just going to assign to this.minRatio anyway in the bottom of the function

This variable is needed for the condition in the bottom:
this.minRatio = minRatio ? minRatio : this.defaultMinRatio;
see updated code

app/code/Magento/Ui/view/base/web/js/grid/masonry.js

DrewML · 2019-11-05T20:15:38Z

General question about the technical design of this feature: what's the reasoning for doing all the calculations in JavaScript and resizing, rather than using a mostly CSS approach? Were there constraints due to browser support, or something else?

app/code/Magento/Ui/view/base/web/js/grid/masonry.js

sivaschenko · 2019-11-06T16:14:32Z

@DrewML we were not able to find a responsible and flexible solution in plain CSS

sivaschenko · 2019-11-06T16:57:00Z

Thanks for the reviews @omiroshnichenko and @DrewML .

The suggested improvements have been applied. Please review.

magento-engcom-team · 2019-11-06T20:11:33Z

Hi @VladimirZaets, thank you for the review.
ENGCOM-6244 has been created to process this Pull Request

m2-assistant · 2019-11-08T04:02:39Z

Hi @sivaschenko, 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.

VladimirZaets · 2019-11-18T17:07:32Z

These components use by Adobe Stock project and covered by functional tests.

Added masonry grid and columns components

81d2758

sivaschenko requested review from DrewML and melnikovi as code owners November 4, 2019 15:20

magento-engcom-team added Area: Frontend Component: Ui labels Nov 4, 2019

magento-engcom-team added the Release Line: 2.3 label Nov 4, 2019

sivaschenko added the Auto-Tests: Not Required Changes in Pull Request does not require coverage by auto-tests label Nov 4, 2019

sivaschenko requested review from YevSent, omiroshnichenko and sidolov November 4, 2019 15:20

#25464: Added unsanitized html suffix to the error message html

c0119d9

YevSent approved these changes Nov 4, 2019

View reviewed changes

omiroshnichenko reviewed Nov 4, 2019

View reviewed changes

app/code/Magento/Ui/view/base/web/js/grid/columns/image-preview.js Outdated Show resolved Hide resolved

omiroshnichenko reviewed Nov 4, 2019

View reviewed changes

app/code/Magento/Ui/view/base/web/js/grid/columns/image.js Outdated Show resolved Hide resolved

omiroshnichenko reviewed Nov 4, 2019

View reviewed changes

app/code/Magento/Ui/view/base/web/js/grid/columns/overlay.js Outdated Show resolved Hide resolved

omiroshnichenko reviewed Nov 4, 2019

View reviewed changes

app/code/Magento/Ui/view/base/web/js/grid/masonry.js Outdated Show resolved Hide resolved

omiroshnichenko reviewed Nov 4, 2019

View reviewed changes

app/code/Magento/Ui/view/base/web/templates/grid/columns/image-preview.html Outdated Show resolved Hide resolved

omiroshnichenko reviewed Nov 4, 2019

View reviewed changes

app/code/Magento/Ui/view/base/web/templates/grid/columns/image-preview.html Outdated Show resolved Hide resolved

omiroshnichenko reviewed Nov 4, 2019

View reviewed changes

app/code/Magento/Ui/view/base/web/templates/grid/masonry.html Outdated Show resolved Hide resolved

omiroshnichenko reviewed Nov 4, 2019

View reviewed changes

#25464: Applied code review suggestions

c657fb2

omiroshnichenko reviewed Nov 4, 2019

View reviewed changes

app/code/Magento/Ui/view/base/web/js/grid/columns/image-preview.js Outdated Show resolved Hide resolved

sivaschenko added 2 commits November 5, 2019 11:19

#25464: Applied code review suggestions

5aebbc0

#25464: Corrected annotation

02dfa0d

DrewML reviewed Nov 5, 2019

View reviewed changes

app/code/Magento/Ui/view/base/web/js/grid/masonry.js Outdated Show resolved Hide resolved

DrewML reviewed Nov 5, 2019

View reviewed changes

app/code/Magento/Ui/view/base/web/js/grid/masonry.js Show resolved Hide resolved

omiroshnichenko reviewed Nov 5, 2019

View reviewed changes

app/code/Magento/Ui/view/base/web/js/grid/masonry.js Outdated Show resolved Hide resolved

#25464: Applied code review suggestions

0e5eaaf

sivaschenko added 2 commits November 6, 2019 17:59

Visible record should not be stateful

4787165

#25464: Fixed static tests

2c0f863

VladimirZaets approved these changes Nov 6, 2019

View reviewed changes

ghost assigned VladimirZaets Nov 6, 2019

ghost added the Progress: ready for testing label Nov 6, 2019

sivaschenko added 2 commits November 6, 2019 20:29

#25464: Single quotes are redundant for object keys

ea18bd7

Added raf untility for window resizing

6023ac7

ghost added Progress: accept and removed Progress: ready for testing labels Nov 6, 2019

DrewML approved these changes Nov 6, 2019

View reviewed changes

diazwatson requested a review from omiroshnichenko November 6, 2019 23:27

omiroshnichenko approved these changes Nov 6, 2019

View reviewed changes

magento-engcom-team merged commit 6023ac7 into magento:2.3-develop Nov 8, 2019

magento-engcom-team added this to the Release: 2.3.4 milestone Nov 8, 2019

konarshankar07 mentioned this pull request Nov 17, 2019

Masonry component doc added magento/devdocs#6013

Merged

VladimirZaets added Auto-Tests: Covered All changes in Pull Request is covered by auto-tests and removed Auto-Tests: Not Required Changes in Pull Request does not require coverage by auto-tests labels Nov 18, 2019

konarshankar07 mentioned this pull request Nov 19, 2019

Image preview component added magento/devdocs#6028

Merged

Added masonry grid and columns components #25464

Added masonry grid and columns components #25464

Uh oh!

Conversation

sivaschenko commented Nov 4, 2019

Description (*)

Contribution checklist (*)

Uh oh!

m2-assistant bot commented Nov 4, 2019

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

omiroshnichenko Nov 4, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

omiroshnichenko Nov 4, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

sivaschenko commented Nov 5, 2019

Uh oh!

DrewML Nov 5, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

DrewML commented Nov 5, 2019

Uh oh!

Uh oh!

sivaschenko commented Nov 6, 2019

Uh oh!

sivaschenko commented Nov 6, 2019

Uh oh!

magento-engcom-team commented Nov 6, 2019

Uh oh!

m2-assistant bot commented Nov 8, 2019

Uh oh!

VladimirZaets commented Nov 18, 2019

Uh oh!

Uh oh!

omiroshnichenko Nov 4, 2019 •

edited

Loading

omiroshnichenko Nov 4, 2019 •

edited

Loading

DrewML Nov 5, 2019 •

edited

Loading