MHemsley
commented
7 years ago Hey Eoin,
Thanks for doing this, it provides an incredibly easy way to work through a defragmentation process, which can often feel incredibly daunting.
I would certainly recommend dealing with these issues first, being someone who despises refactoring, I got incredibly cocky on previous projects and that’s really bitten me in the butt. The refactoring would also fit in with the priorities for this first sprint that we talked about last week.
Best wishes, Mark
On 6 Mar 2017, at 09:07, Eoin McCarthy notifications@github.com wrote:
Hi I hope you can appreciate the time constraints last time meant that we are not quite where we want to be in terms of code quality. Although I am not sure what you will keep here it is easiest for me just to look at it all now. I would expect you to break each of these points into its own issue before working on it. You can drop the issue num into each point as you go so you have a clear top level.
All files in root. You should only keep top level config files / entry points to application in the root of a project. Right now all you js / html / css is in the root of your project. I appreciate that your html needs to live at a path that matches its url, but you have no such constraints on js / css use node for deps. Now that you are building a server, you can remove jquery from your source. Organise js. Right now you have js files using different patterns for containing functionality. We have: Everything written into the root of the file (don't do this!); everything wrapped in jquery document.ready function (this is fine); everything wrapped in iife (this is also fine). You should organise your js! Having a bunch of different scripts lying around with different invocation patterns in the root of you project is not good... Either have zero / one js file per page (clearly named) or have just one js file. fix this file:
https://github.com/CYPIAPT-LNDSE/welcome-to-camhs/blob/master/swipe.js. This code is not dry. You should be able to define an object holding data, then a function to manage this branching. Please ask me if this does not make sense. Same with this file
https://github.com/CYPIAPT-LNDSE/welcome-to-camhs/blob/master/balloon.js. remove comment here:
https://github.com/CYPIAPT-LNDSE/welcome-to-camhs/blob/master/main.css#L265 main.css looks nice!
Okay I am sure this list is not complete but this is stuff that jumped out at me. I hope this all makes sense to you. If I ever seem short / abrupt in gh / gitter it does not mean I am annoyed or anything! Please know you are always welcome to chat to me on hangouts / gitter. On the whole, if I see any pulls changing things I have brought up here but where it has not been fixed, I will ask you to fix it before approving the pull, but it is up to you whether you tackle all together at beginning or fix things as you touch them. So please don't start submitting pulls touching this code unless they also address relevant comments! Hope london is nice! — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub, or mute the thread.
{"api_version":"1.0","publisher":{"api_key":"05dde50f1d1a384dd78767c55493e4bb","name":"GitHub"},"entity":{"external_key":"github/CYPIAPT-LNDSE/welcome-to-camhs","title":"CYPIAPT-LNDSE/welcome-to-camhs","subtitle":"GitHub repository","main_image_url":"https://cloud.githubusercontent.com/assets/143418/17495839/a5054eac-5d88-11e6-95fc-7290892c7bb5.png","avatar_image_url":"https://cloud.githubusercontent.com/assets/143418/15842166/7c72db34-2c0b-11e6-9aed-b52498112777.png","action":{"name":"Open in GitHub","url":"https://github.com/CYPIAPT-LNDSE/welcome-to-camhs"}},"updates":{"snippets":[{"icon":"DESCRIPTION","message":"Code review (#58)"}],"action":{"name":"View Issue","url":"https://github.com/CYPIAPT-LNDSE/welcome-to-camhs/issues/58"}}}
This email has been scanned for email related threats and delivered safely by Mimecast. For more information please visit http://www.mimecast.com
RhodesPeter
skibinska
Jasminpatel1
bradreeder
Hi I hope you can appreciate the time constraints last time meant that we are not quite where we want to be in terms of code quality. Although I am not sure what you will keep here it is easiest for me just to look at it all now.
I would expect you to break each of these points into its own issue before working on it. You can drop the issue num into each point as you go so you have a clear top level.
document.readyfunction (this is fine); everything wrapped in iife (this is also fine). You should organise your js! Having a bunch of different scripts lying around with different invocation patterns in the root of you project is not good... Either have zero / one js file per page (clearly named) or have just one js file.Okay I am sure this list is not complete but this is stuff that jumped out at me. I hope this all makes sense to you. If I ever seem short / abrupt in gh / gitter it does not mean I am annoyed or anything! Please know you are always welcome to chat to me on hangouts / gitter.
On the whole, if I see any pulls changing things I have brought up here but where it has not been fixed, I will ask you to fix it before approving the pull, but it is up to you whether you tackle all together at beginning or fix things as you touch them. So please don't start submitting pulls touching this code unless they also address relevant comments!
Hope london is nice!