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.

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

[2.3] Reworked gallery.phtml to move generation of gallery json strings to own block functions #18440

Merged
merged 5 commits into from
Apr 8, 2019
Merged

Conversation

gwharton
Copy link
Contributor

@gwharton gwharton commented Oct 7, 2018

Description

Reworked gallery.phtml to move generation of gallery json strings to own block functions
Introduced unit tests to cover generation of gallery json strings with full type checking.

The resultant code is much easier to diagnose and test. It uses the json_encode and json_decode routines instead of the old manual way of generating json strings that were used in the old gallery.phtml which is prone to error and mistakes.

Fixed Issues (if relevant)

  1. none. Rework and unit tests.

Manual testing scenarios

  1. View source on any product page.
  2. Ensure the gallery section has fully formed json for gallery and gallery fullscreen sections.

Stock Magento should be

"options": {"nav":"thumbs","loop":true,"keyboard":true,"arrows":true,"allowfullscreen":true,"showCaption":false,"width":700,"thumbwidth":88,"thumbheight":110,"height":700,"transitionduration":500,"transition":"slide","navarrows":true,"navtype":"slides","navdir":"horizontal"}

and

"fullscreen": "nav":"thumbs","loop":true,"navdir":"horizontal","navarrows":false,"navtype":"slides","arrows":true,"showCaption":false,"transitionduration":500,"transition":"slide"}

All strings/bools/ints are now typed properly as per the Magento dev docs for the Magento 2 Gallery Widget.

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)

@gwharton
Copy link
Contributor Author

gwharton commented Oct 7, 2018

The 2.2 Backport PR is here #18443

@ihor-sviziev ihor-sviziev self-assigned this Oct 8, 2018
app/code/Magento/Catalog/Block/Product/View/Gallery.php Outdated Show resolved Hide resolved
app/code/Magento/Catalog/Block/Product/View/Gallery.php Outdated Show resolved Hide resolved
@gwharton gwharton dismissed sidolov’s stale review November 10, 2018 19:35

Changes incorporated

@magento-engcom-team
Copy link
Contributor

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

@sidolov
Copy link
Contributor

sidolov commented Mar 5, 2019

Hi @gwharton , it's a really good changes that helps external developers customize gallery configuration, but unfortunately, we are not allowed to add new public methods to blocks that marked as @api.
As alternative solution you may extract logic to the separate class, inject it to block via di configuration and use in the phtml template. You can find the example in the /app/code/Magento/Sales/view/adminhtml/templates/order/create/form/address.phtml
Thank you!

@gwharton
Copy link
Contributor Author

gwharton commented Mar 6, 2019

@sidolov, Thats neat. I hadn't realised you could do that.

I've updated the 2.3 and 2.2 PR's accordingly. Just waiting on the Travis backlog, but pretty sure they should come in green. If not I will resolve.

@magento-cicd2
Copy link
Contributor

CLA assistant check
Thank you for your submission, we really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.

@gwharton
Copy link
Contributor Author

Just waiting on a travis pass.

I have reverted all changes to Gallery.php and GalleryTest.php as all active changes were moved to GalleryOptions.php and GalleryOptionsTest.php and the changes that were left in the original Gallery.php and GalleryTest.php were cleanup code unrelated to this PR and a distraction.

This PR feels much cleaner now.

@magento-engcom-team
Copy link
Contributor

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

@magento-engcom-team
Copy link
Contributor

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

@VasylShvorak
Copy link
Contributor

✔️ QA passed

@m2-assistant
Copy link

m2-assistant bot commented Apr 8, 2019

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

@magento-engcom-team magento-engcom-team added this to the Release: 2.3.2 milestone Apr 8, 2019
magento-engcom-team pushed a commit that referenced this pull request Apr 8, 2019
@gwharton gwharton deleted the 2.3-develop-gallery-rework branch April 23, 2019 20:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants