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

local themes should be added to git repo #18960

Merged
merged 1 commit into from
Nov 22, 2018
Merged

local themes should be added to git repo #18960

merged 1 commit into from
Nov 22, 2018

Conversation

torhoehn
Copy link
Contributor

Removed local-themes.js from .gitignore

Description (*)

Since local-themes.js is always created manually by copying themes.js it should be added to git. So themes.js could also be a .js.dist-file which has to be copied and would be added to git, too.

Fixed Issues (if relevant)

  1. dev/tools/grunt/configs/themes.js gets replaced after update magento #18949: dev/tools/grunt/configs/themes.js gets replaced after update magento

Manual testing scenarios (*)

  1. Copy dev/tools/grunt/configs/themes.js to dev/tools/grunt/configs/local-themes.js
  2. local-themes.js should be found by git now

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)

@magento-engcom-team
Copy link
Contributor

Hi @torhoehn. 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-engcom-team
Copy link
Contributor

Hi @ihor-sviziev, thank you for the review.
ENGCOM-3338 has been created to process this Pull Request

@VladimirZaets
Copy link
Contributor

Hi @torhoehn. Thanks for collaboration. Make local-themes.js as dist file doesn't have a sense, it will be the same with the case if we make the origin themes.js file as dist. Currently, we don't depend to name of custom theme file at all, we can provide any name and any path to custom theme file in the grunt-config.json file that provided out of the box with Magento as dist file.

There is the documentation

If you have some concerns or question feel free to ask. Thanks.

@torhoehn
Copy link
Contributor Author

torhoehn commented Nov 5, 2018

@VladimirZaets I don't want to add .dist-file. It was just an example that the themes.js will be copied to local-themes.js, just like a .dist-file, which has to be copied to be in version control. So my commit will only change that the local-themes.js will be in version control.

@ihor-sviziev
Copy link
Contributor

@VladimirZaets we don't want to add this file to magento2 repo, but when it's m2 project - for this project you should be able to commit local-themes.js file into own repo, it shouldn't be ignored.

@magento-engcom-team
Copy link
Contributor

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

@magento-engcom-team
Copy link
Contributor

Hi @torhoehn. Thank you for your contribution.
We will aim to release these changes as part of 2.2.8.
Please check the release notes for final confirmation.

Please, consider to port this solution to 2.3 release line.
You may use Porting tool to port commits automatically.

@torhoehn torhoehn deleted the feature/grunt-local-themes branch November 22, 2018 18:44
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.

5 participants