Good morning P. and P.
As we we couldn’t catchup yesterday and I couldn’t feed you with proper feedback, then I promised to write you an open letter with all the points that I wanted to discuss.
First of all thanks for the git pull request and giving me the chance to review your code. I have to say that in general I am quite impressed. You managed to “draw the line” where is the scope your work and you properly marked the places inside the templates where the backend team have to step in. The idea with the TODOs and the timestamp was really original. This will make the things for us much more easier in the near future, when we will replace your static texts with our data getters.
There was something specific for your pull request. It contained many commits and it was representing your work from the last weeks. I noticed some small issues that were repeated in different template files.
I like to give you my honest feedback in order mistakes like these to be minimised in the in the future:
1. Missing TODO comments:
The idea with the TODOs is great and it really makes sense, but I noticed that at some places where we had temporary hardcoded text the TODO comment was missing. Probably you forgot to put the comment or you were distracted by something, but I hope that we can minimise the number of these mistakes.
2. There is no sense to use German text inside the “__()” function:
I recommend inside the templates to use the English text inside the translate function and later this string to be translated from the translation CSV files. For example “$this->__(‘Menge:’)” is wrong. The correct format is “echo $this->__(‘Qty:’)”.
3. Do not copy .phtml files that you are not going to modify:
When you copy a file from the parent theme in our own theme always make sure that you are going to change this file, because otherwise it doesn’t make sense. I noticed some files that were copied from the parent theme, but were not changed … I can assume that you had in mind to change them, but during the process you abandoned your idea. A possible solution is to compare the files that are about to be committed with the files from the parent theme aka. the fallback theme. In that way you can easily spot if the file should be committed.
4. Missing file definitions in the modman file:
Always check if there is a modman definition for the files that are about to be committed. You can check the modman file and to check if the files are symlinked in Magento as well.
5. Wrong formatting of the inline PHP code and JS code inside the .phtml files:
I noticed that the formatting in some files was changed, but there were no changes in the logic. Basically this makes reviewing the pull request a bit harder. I can assume that this formatting was “broken” when you copied the text content from a file in to newly created file ( aka. using the clipboard ). I can suggest always to copy the physical file instead of copy / pasting the content. Another assumptions is that your text editor is messing up the formatting.
That’s it so far. Just turn back and smile
Your thoughts / questions?