Skip to content

#1981 Allow inclusion of a file dev/tools/grunt/themes.local.js #1982

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 1 commit into from

Conversation

amenk
Copy link
Contributor

@amenk amenk commented Sep 29, 2015

  • include local theme configuration, so this can be comited to the local git and shared between developers
  • I choose the path grunt instead of grunt/config because it would be bothersome to exclude all standard grunt configs

@amenk
Copy link
Contributor Author

amenk commented Sep 30, 2015

Unfortunately grunt/local is deleted after composer update

@magento-cicd2
Copy link
Contributor

We have automated a Magento Contributor License Agreement verifier for contributions sent to our GitHub projects.
Please see the CLA agreement in the Pull Request comments below.

@guz-anton
Copy link
Contributor

Hi Alexander,
Sorry, but I can't accept you changes.
Currently configs/theme.js is just an example of structure. We expect that developer works with own themes. So he should rewrite content of configs/theme.js with own values.

Pros :

  • During active development with own theme you don't need waste time for rebuild in-box themes.

Cons :

  • This file is under git. Should be not so suitable to rewrite it. But it depend on your workflow.

@amenk
Copy link
Contributor Author

amenk commented Oct 2, 2015

Yes - the con is the problem. It is deployed via the magento framework composer package, so I can not change it in my git. What about moving the theme.js to app/etc/ and supply a theme.example.js ?

@guz-anton
Copy link
Contributor

moving the theme.js to app/etc/ and supply a theme.example.js

I think its good point.

@amenk
Copy link
Contributor Author

amenk commented Oct 2, 2015

okay shall I make a PR ?

@guz-anton
Copy link
Contributor

Yes. You are welcome :)

@amenk
Copy link
Contributor Author

amenk commented Oct 3, 2015

Option 1:
What do you think - should we just delete the dev/tools/grunt/configs/themes.js and replace it with a app/etc/themes.js? That would mean that by default grunt will not work and the dev has to copy the app/etc/themes.template.js to app/etc/themes.js.
Would this break something else?

Options 2:
Just the same way I do it currently, extend the standard themes.js by the local one.

Option 3:
If app/etc/themes.js is present, then ignore the dev/tools/grunt/configs/themes.js

Both options are more complex to implement (okay, 2 more lines). So I would vote for Option 1.

Please comment.

* themes.js is no longer published with the project. So it will not be overwritten
  on composer updates of the Magento framework
* Original themes.js becomes an example/template
* themes.js is managed to app/etc - just like env.php
@amenk
Copy link
Contributor Author

amenk commented Oct 3, 2015

I have implemented the option 1. Actually I force-pushed it to the patch branch, so my old approach was removed from the history. Sorry.

@twiking
Copy link

twiking commented Mar 7, 2016

Any update on this one or has it been solved in another way @guz-anton?

@guz-anton
Copy link
Contributor

Hi Tobias,
Still no updates. This issue is team backlog. We don't miss it. But it still needs internal discussion.

I'll notify here in case any changes.

@guz-anton
Copy link
Contributor

Notice about .gitignore - you can exclude helper files locally in any repo. More

$  echo '/app/etc/theme.js' >> .git/info/exclude

@matt-bailey
Copy link

Any updates on this? themes.js being overwritten every time we composer install/update is a problem as we need to be able to include our own custom themes in it.

@hostep
Copy link
Contributor

hostep commented Jun 3, 2016

cross-reference with: #4697

Not sure what the best solution between the two options is though.

@guz-anton
Copy link
Contributor

Just a notice: internal ticket MAGETWO-55345 has been created.

@sshrewz
Copy link

sshrewz commented Aug 11, 2016

@amenk, we need you to sign the CLA before we can proceed with further actions on your contribution.

@amenk
Copy link
Contributor Author

amenk commented Aug 12, 2016

@Snohe I signed that thing a dozen times :-) How to sign it now?

@hostep
Copy link
Contributor

hostep commented Aug 12, 2016

@Snohe: this PR tries to accomplish the same as another accepted PR but in a different way (I think): #4716, so be careful what you do please :)

@vkorotun vkorotun added Issue: Ready for Work Gate 4. Acknowledged. Issue is added to backlog and ready for development and removed linked labels Aug 22, 2016
@guz-anton
Copy link
Contributor

From now you have ability to place local
dev/tools/grunt/configs/themes.loc.js config file.

See implementation for more details.

Thanks for report, ideas and insights.

@guz-anton guz-anton closed this Sep 5, 2016
magento-team pushed a commit that referenced this pull request Jan 23, 2018
…ta-migration

[Obsessive-Owls] MAGETWO-70174: fix test xml files
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: Frontend improvement Issue: Ready for Work Gate 4. Acknowledged. Issue is added to backlog and ready for development
Projects
None yet
Development

Successfully merging this pull request may close these issues.