Changes to make the gem a bit more modular #96
Conversation
|
@devonlambert Thanks for the PR, will check it and let you know if I have any thoughts. |
| @@ -724,8 +724,8 @@ | |||
| cancel : false, | |||
| cancelClass : 'raty-cancel', | |||
| cancelHint : 'Cancel this rating!', | |||
| cancelOff : '<%= asset_path('cancel-off.png') %>', | |||
| cancelOn : '<%= asset_path('cancel-on.png') %>', | |||
| cancelOff : 'cancel-off.png', | |||
There was a problem hiding this comment.
Can you explain why did you remove the asset_path?
There was a problem hiding this comment.
@devonlambert I think you should use asset_path here, otherwise it will not uses the asset pipeline to render the image
|
@devonlambert Great that you splitted the PR into atomic commits, but just a small thing to mention, please change the commit messages to be in an imperative manner, like: To stick to the convention of Git commit messages. Thanks again for your PR, and your time. |
The original file setup pushed all of the Gem's assets into the application's main asset folder. By moving these into subfolders, there is less chance that a user will lose track of the assets, overwrite their own assets or have the assets interfere with the asset organization for the site.
…c defaults into ratyrate.js.erb The original jquery.raty plugin/script has a defaults object that was being overwritten by the ratyrate dev team. One of the settings was adding asset-path within this file. However, this file is not an ERB file so Rails was failing when it hit it. By pulling that code from the plugin and instead placing it in the ratyrate.js.erb file we are abstracting it out of the plugin. This fixes the issue that Rails has AND it also means that if the raty.js script is updated we could easily drop in the new version without worrying about any the defaults we needed for the ratyrate Gem.
|
Hi @wazery, I've added additional information to the commits and updated the subject lines to use the imperative manner. In short, the changes I am proposing should make it easier to drop in newer versions of the Raty.js script as it is updated. The way you have it today means that if a new Raty.js version is introduced the "defaults" method would be overridden. Also, Rails was not able to work out the asset_path call in the Raty.js file because you were not using an ERB file. |
| copy_file 'mid-star.png', 'app/assets/images/stars/raty/mid-star.png' | ||
| copy_file 'big-star.png', 'app/assets/images/stars/raty/big-star.png' | ||
| copy_file 'cancel-on.png', 'app/assets/images/stars/raty/cancel-on.png' | ||
| copy_file 'cancel-off.png', 'app/assets/images/stars/raty/cancel-off.png' |
There was a problem hiding this comment.
@devonlambert are you trying to change the location of the icons? If so I think u need to update the location in the rendering files...
There was a problem hiding this comment.
@yihyang I am trying to change the location of the icons. However I didn't believe there was a need to change the rendering file as Rails Asset pipeline is smart enough to work out the "asset_path"?
Isn't that the case?
The changes I've made allow for a bit more modularity of the generated code from the Gem. Wanted to share the changes and see what you thought.