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

Fixed js dependencies in the blank theme #10916

Closed
wants to merge 1 commit into from

Conversation

rogyar
Copy link
Contributor

@rogyar rogyar commented Sep 17, 2017

Description

There's an issue with creating a new theme that does not have blank theme in the fallback. The RequireJs is looking for the following dependencies:

  • js/responsive
  • js/theme

in the theme's web directory. The mentioned JS files are present in the blank theme but not in the newly created theme. As result, RequireJs throwing a couple of errors and JS logic can be broken on some pages (checkout) because of that.
The fix moves the dependencies from the global theme scope to the Magento_Theme RequireJS config of the blank theme. In that way, the blank theme still have the mentioned dependencies. But a clean theme without blank theme in the fallback will not request these dependencies.
The similar approach is used for further Magento versions in the develop branch.

Fixed Issues

  1. 2.1.7 - Checkout loader is not removed after page load using empty theme #10662: 2.1.7 - Checkout loader is not removed after page load using empty theme

Manual testing scenarios

Current behaviour

  1. Create and enable a clean theme without a parent theme
  2. Add some product to the shopping cart and go to checkout
  3. You will get few errors from RequireJS and the checkout loading animation never disappears

Fixed behaviour

  1. Create and enable a clean theme without a parent theme
  2. Add some product to the shopping cart and go to checkout
  3. All checkout components should be loaded without issues

@orlangur
Copy link
Contributor

@rogyar please force push a branch so that it does not contain changes from #10931.

@rogyar
Copy link
Contributor Author

rogyar commented Sep 18, 2017

@orlangur done, missed that point, thank you.

@vkublytskyi
Copy link

Despite the fact that this PR is a backport of #8982 after discovering the content of responsive.js and theme.js and internal discussion we came to a conclusion that this PR introduces issue with code modularity and can't be accepted as is (as well as changes made by original PR should be modified).

The problem is that JS files operate with HTML nodes introduced by not a Magento_Theme module (e.g. #product-info-additional, #block-shipping, etc.). Such code introduces unnecessary coupling between modules. As increasing modularity for Magento 2 is a high priority now fix should be aligned with this goal. Therefore js files should be placed under different Folders inside theme or even split into several smaller files with single responsibility and straight relation to a particular module.

@rogyar Let us know are you interested to continue work on this issue according to the described goal or this task should be moved to Up for Grabs board.

@rogyar
Copy link
Contributor Author

rogyar commented Oct 7, 2017

@vkublytskyi the main purpose of this PR was to fix the issue using the same (or similar) approach as it used in the core at the moment. That's why the architecture changes are not taken into account here. So, changing the approach is more like another PR with the architecture improvements purpose but not a bugfix.
Anyway, if you think that it should be a part of this PR, I'm glad to help. Just want to ensure that we are on the same page. We can move particular parts of the responsive.js and theme.js to the separate files in different modules. For example the following part:

if ($('body').hasClass('checkout-cart-index')) {
        if ($('#co-shipping-method-form .fieldset.rates').length > 0 &&
            $('#co-shipping-method-form .fieldset.rates :checked').length === 0
        ) {
            $('#block-shipping').on('collapsiblecreate', function () {
                $('#block-shipping').collapsible('forceActivate');
            });
        }
    }

could be placed into a new JS file located in the Magento_Checkout theme module. In the same way, the JS logic that is responsible for product additional information could be placed in the Magento_Catalog theme module. What do you think?

Thank you.

@vkublytskyi
Copy link

@rogyar, sorry for the delay with response. Proposed changes look relevant. We are trying to treat JS modules as public API so moving them is a backward incompatible change. That's why it's better to do this change in one PR.

@okorshenko okorshenko modified the milestones: October 2017, November 2017 Nov 1, 2017
@magento-engcom-team magento-engcom-team added bugfix Fixed in 2.2.x The issue has been fixed in 2.2 release line labels Nov 7, 2017
@vkublytskyi
Copy link

Hi @rogyar. Are you going to continue work on this PR?

@ishakhsuvarov
Copy link
Contributor

Hi @rogyar
We are closing this PR now due to inactivity. Please reopen if you wish to continue working on it.
Thank you.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants