Skip to content

[2.2] Implement alternative fix for #12285:The option false for mobile device don't work in product view page gallery #17969

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

Closed
wants to merge 11 commits into from

Conversation

gwharton
Copy link
Contributor

@gwharton gwharton commented Sep 7, 2018

Description

The original fix for #12285 made changes to the way that getVar returns variables from the view.xml file. The original fix changed it so that if the variable content was 'false' or 'true' it would return a boolean value of false or true instead of returning the string value of the boolean as it did before.

Unfortunately this change means that you can no longer tell the difference between the variable existing and being set to false, and the variable not actually existing.

A call to getVar for any random variable name will return false if it doesn't exist, and also false if it happened to exist and was set to 'false'.

This is not good.

This change reverts that change so that the function returns boolean false if the variable name does not exist, and returns the string 'false' if it exists and is set to false. It will always return the string value of the variable contents regardless of the type.

I have reimplemented the fix in a different way, parsing recursively through the Breakpoints array, changing any string 'true' to boolean true and string 'false' to boolean false before parsing into json for sending to browser. This ensures the original #12285 issue remains fixed.

Fixed Issues (if relevant)

Alternative fix for issue

  1. The option <var name="allowfullscreen">false</var> for mobile device don't work in product view page gallery #12285:The option false for mobile device don't work in product view page gallery

Manual testing scenarios

edit file /etc/view.xml
add <var name="allowfullscreen">false</var> to mobile breakpoints

Example:

   <var name="breakpoints">
            <var name="mobile">
                <var name="conditions">
                    <var name="max-width">767px</var>
                </var>
                <var name="options">
                    <var name="options">
                        <var name="navigation">dots</var>
                        <var name="allowfullscreen">false</var>
                    </var>
                </var>
            </var>
        </var>

clean cache and open source html
find plugin "mage/gallery/gallery" init options on html page.

Check that the option "allowfullscreen" value is being output as boolean and not string.

"breakpoints": {"mobile":{"conditions":{"max-width":"767px"},"options":{"options":{"navigation":"dots","allowfullscreen":false}}}} }

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 added 5 commits September 7, 2018 13:49
…r mobile device don't work in product view page gallery"

This reverts commit b8232c5.
…r mobile device don't work in product view page gallery"

This reverts commit bf40afa.
…r mobile device don't work in product view page gallery"

This reverts commit ccc35c2.
@magento-engcom-team
Copy link
Contributor

Hi @gwharton. 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-engcom-team give me test instance - deploy test instance based on PR changes
  • @magento-engcom-team give me {$VERSION} instance - deploy vanilla Magento instance

For more details, please, review the Magento Contributor Assistant documentation

@magento magento deleted a comment from magento-engcom-team Sep 7, 2018
@magento magento deleted a comment from magento-engcom-team Sep 7, 2018
@magento magento deleted a comment from magento-engcom-team Sep 7, 2018
@magento magento deleted a comment from magento-engcom-team Sep 7, 2018
@gwharton
Copy link
Contributor Author

gwharton commented Sep 7, 2018

@magento-engcom-team give me test instance

@magento-engcom-team
Copy link
Contributor

Hi @gwharton. Thank you for your request. I'm working on Magento instance for you

@magento-engcom-team
Copy link
Contributor

Hi @gwharton, here is your new Magento instance.
Admin access: https://pr-17969.engcom.dev.magento.com/admin
Login: admin Password: 123123q

@gwharton
Copy link
Contributor Author

gwharton commented Sep 8, 2018

The changes to the gallery.phtml file in this PR are an interim solution. I propose to rework the gallery.phtml file with full unit testing in PR #17920 once this PR has been processed.

@gwharton
Copy link
Contributor Author

2.3 also requires this change so let me know if you need a 2.3 PR first.

@gwharton gwharton closed this Oct 7, 2018
@gwharton gwharton deleted the 2.2-develop-12285-2 branch January 7, 2019 19:47
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.

3 participants