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

Move blank theme dependencies out of Magento_Theme requirejs-config #8982

Merged

Conversation

mikeoloughlin
Copy link
Contributor

@mikeoloughlin mikeoloughlin commented Mar 23, 2017

Description

Creating a new theme that does not fallback to the blank theme causes two files to 404 because Magento_Theme module depends on blank theme assets js/theme.js and js/responsive.js

Fixed Issues (if relevant)

  1. js/theme & js/responsive files in wrong requirejs-config file? #3784: js/theme & js/responsive files in wrong requirejs-config file?

Reproduce bug:

  1. Follow the steps to create a new theme from the devdocs
  2. Don't define a parent theme:
<theme xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" xsi:noNamespaceSchemaLocation="urn:magento:framework:Config/etc/theme.xsd">
     <title>New theme</title>
 </theme>
  1. Clear cache and go to the frontend to see the 404s

Manual testing steps:

  1. New theme should no longer attempt to load theme.js or responsive.js
  2. Switch to blank theme and clear cache. Storefront should load theme.js and responsive.js

@magento-cicd2
Copy link
Contributor

magento-cicd2 commented Mar 23, 2017

CLA assistant check
All committers have signed the CLA.

@@ -0,0 +1,11 @@
/**
Copy link
Contributor

@Igloczek Igloczek Mar 26, 2017

Choose a reason for hiding this comment

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

I'd say that adding requirejs-config.js at theme root should be considered as anti-pattern and Magento core shouldn't promote it, b/c it's against "everything is a module" aproach.

By default this config file is stored in Theme module of Magento vendor, so following files structure, it should be added as app/design/frontend/Magento/blank/Magento_Theme/requirejs-config.js and JS files should be moved from app/design/frontend/Magento/blank/web/js to app/design/frontend/Magento/blank/Magento_Theme/web/js.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's a good point and I agree with you. I've updated the code and tested it.

Thanks :)

@ishakhsuvarov ishakhsuvarov self-assigned this Mar 27, 2017
@ishakhsuvarov ishakhsuvarov added this to the March 2017 milestone Mar 27, 2017
@magento-team magento-team merged commit f793c25 into magento:develop Mar 31, 2017
magento-team pushed a commit that referenced this pull request Mar 31, 2017
magento-team pushed a commit that referenced this pull request Mar 31, 2017
magento-team pushed a commit that referenced this pull request Mar 31, 2017
@mikeoloughlin mikeoloughlin deleted the blank-theme-requirejs-config branch March 31, 2017 10:32
@okorshenko
Copy link
Contributor

@mikeoloughlin thank you for your contribution!

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.

6 participants