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

Fotorama gallery configuration fix #15546

Conversation

furseyev
Copy link
Contributor

Description

Fotorama gallery configuration fix: Removing check for boolean variables. This check will prevent populating "false" value - default value will be used instead.

Previous code for boolean config looked like:
<?php if (($block->getVar("gallery/caption"))): ?> "showCaption": <?= /* @escapeNotVerified */ $block->getVar("gallery/caption") ?>, <?php endif; ?>
In this case if show caption option set to "false", we will simply won't get the condition body output, so fotorama will use a default configuration.

app/design/frontend/Magento/blank/etc/view.xml has:
<vars module="Magento_Catalog"> <var name="gallery"> <var name="caption">false</var> </var> </vars>
So the default value will be shown:
lib/web/fotorama/fotorama.js line 621:
showcaption: true,
Which means that image caption will be shown always, ignoring theme's configuration.

Manual testing scenarios

  1. In 2.2 develop branch go to the product view page.
  2. Product gallery will have image caption

…les. This check will prevent populating "false" value - default value will be used instead.
@rogyar rogyar self-assigned this May 27, 2018
@magento-engcom-team magento-engcom-team added this to the Release: 2.2.6 milestone May 29, 2018
@magento-engcom-team magento-engcom-team added Release Line: 2.2 Partner: Atwix Pull Request is created by partner Atwix partners-contribution Pull Request is created by Magento Partner Area: Frontend Component: Catalog labels May 29, 2018
@magento-engcom-team
Copy link
Contributor

@furseyev thank you for contributing. Please accept Community Contributors team invitation here to gain extended permissions for this repository.

@rogyar
Copy link
Contributor

rogyar commented Jun 3, 2018

Hello @furseyev. The previous solution is not correct for sure. But in case of the current solution, a default value is never used. If there's no value for some var, it's always 'false', so no fallback to the default value.

Correct me if I'm wrong, please.

@furseyev
Copy link
Contributor Author

furseyev commented Jun 6, 2018

Hello @rogyar ,
If some of the variables are not specified in app/design/frontend/FooVendor/BarTheme/etc/view.xml they will be inherited from the parent theme. Eventually, the inheritance will lead us to the vendor/magento/theme-frontend-blank/etc/view.xml in almost all the cases. So the vendor/magento/theme-frontend-blank/etc/view.xml is an actual source of the default values here.
Though it is not mandatory for any theme to have a parent theme:
vendor/magento/framework/Config/etc/theme.xsd
Themes usually should inherit from blank or luma. Finally, looking at the
\Magento\Framework\View\Element\AbstractBlock::getVar
and further at the
\Magento\Framework\Config\View::getVarValue
we can see that if some config is not set - the method will return boolean "false", so we are not able to determine if the config value was set in the etc/view.xml of the theme or its parents from the template/block context. And were are not able to modify the \Magento\Framework\Config\View::getVarValue as \Magento\Framework\Config\View is marked with an "@api" tag.
Please clarify if anything else should be done here.
Thanks!

@rogyar
Copy link
Contributor

rogyar commented Jun 20, 2018

Hello @furseyev. Thanks, I got your point. However, still have some doubts regarding the correct way.
Let's imagine a scenario when we don't have a value set for gallery/allowfullscreen in the current theme.

Expected behavior:
The value is fetched from a parent theme, if any.

Current behavior:
The value is false since getVar() returns false if the value does not exists.

So, basically, the scenario when we have gallery/allowfullscreen = true in a parent theme and the corresponding value is not set in the child theme is never reflected as gallery/allowfullscreen = true in the child theme. false is the only variant that is returned when there's no value in the child theme, no fallback is happened.

Copy link

@adamj88 adamj88 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're missing commas off the end of all those additions causing invalid JSON error.

The gallery/navarrows value is also missing it's key.

@furseyev
Copy link
Contributor Author

furseyev commented Jul 5, 2018

@adamj88 ,
Thanks, fixed.

@adamj88
Copy link

adamj88 commented Jul 10, 2018

@furseyev worth noting as well, certain values can be different to true and false, for example:

The option <?= /* @escapeNotVerified */ $block->getVar("gallery/arrows") ?> can be true, false or 'always' - see http://fotorama.io/customize/options/#-arrows-small-boolean-small-

Would be worth cross referencing those against the full set of options as it is currently limiting them to true/false.

@miguelbalparda
Copy link
Contributor

Right now we have 3 different solutions for this issue #16594 #15546 #16941. It seems we will be moving forward with #16594 but we'd like to have some more input if possible.

@miguelbalparda
Copy link
Contributor

Closing in favor of #16594

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: Frontend Component: Catalog Partner: Atwix Pull Request is created by partner Atwix partners-contribution Pull Request is created by Magento Partner Progress: needs update Release Line: 2.2
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants