-
Notifications
You must be signed in to change notification settings - Fork 13
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
added configuration in admin to make module reusable #2
Conversation
@@ -2,6 +2,7 @@ | |||
"name": "bitbull/magento2-module-instagramwidget", | |||
"description": "Instagram stream for Magento2", | |||
"license": "proprietary", | |||
"version": "1.0.1", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why did you choose to put it here? So we must always keep it aligned with our composer.json. There is a reason for doing so?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added because if installing through composer, it needs a version when running composer update
to pull in latest changes, so should also add version tagging if possible. But can change to whatever version you like.
So if tagging with version, could install a specific version with composer, ie: composer require "bitbull/magento2-module-instagramwidget:1.0.1"
To clarify, module.xml specifies setup version for sql updates, while composer.json could be small update for bugfix.
See: https://magento.stackexchange.com/questions/127967/version-number-in-custom-modules-module-xml-and-composer-json
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok understand the problem: when I moved this repository from bitbucket to github I forgot the tags. Generally we prefer to tag the repo instead of putting the tag in the composer.json of the module. Do you agree?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed.
"InstagramWidget": "Bitbull_InstagramWidget/js/InstagramWidget" | ||
map: { | ||
'*': { | ||
InstagramWidget: 'Bitbull_InstagramWidget/js/InstagramWidget' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why in your opinion I can't only specify the path?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
According to devdocs, this is how it should be implemented in requirejs config.
See:
http://devdocs.magento.com/guides/v2.1/javascript-dev-guide/javascript/custom_js.html
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, devdocs says so but according with amdjs documentation it seems more correct path directive because here I'm just assigning a path to a module id. What do you think? https://github.com/amdjs/amdjs-api/wiki/Common-Config#map-
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi again, I couldn't find an official answer from M2 team on why map is the preference on M2, but from this link my understanding is map is the correct way to go.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm unsure. For the moment I leave the way you did, then I'll open a thread in the forum
also changed path > map in requirejs