Skip to content

Conversation

@lucasconstantino
Copy link
Contributor

I've made some refactoring on front-end code to mostly add JSDoc comments and document some of the code. There is still one file to go: controllers.js. This is much harder to achieve, as it contains most of the front-end logic and thus should probably be splitted in many files.

As commit afeab9b made some mutual changes to the app.js file that resulted in conflicts, I prefered to make a rebase of my code to roll my changes on top of your code, as this was much easier to do, more organized and resulted in this pull-request beeing able to be merged to master with no effort from your side. That's why I suffixing my branch with "rebase".

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can use line comments (//) for this comment here and the param doesn't need to be documented, since the function is an ordinary callback and doesn't declare anything. For directive, class methods, etc we can/should use block comments.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, that's something I get myself thinking all the time when using callback functions declared inline. I supposed you are right, I'll change it. By the way: I've commented the block it self because on my applications I tend not to do all the job in only one config, but to separate and create one config statement for different concerns.

@recidive
Copy link
Owner

Nice refactoring @lucasconstantino!

I believe they are all good, just few code-style related issues, I can do them myself if you prefer, but thought it would be good to let you know.

Have you tested and confirmed everything is fine? At this stage I think it's ok to don't have other person review it if you say it's running fine.

@lucasconstantino
Copy link
Contributor Author

Thanks ;)

So, I can make the refactoring tomorrow. I believe it's better that I do it, so that the commits can come to this pull request to make it clean and done.

Although most of the changes are only documentation and organization of code, I'm constantly testing the application, and as far as I've noticed nothing has broken. Yet :)

lucasconstantino pushed a commit to TallerWebSolutions/choko that referenced this pull request May 27, 2014
lucasconstantino pushed a commit to TallerWebSolutions/choko that referenced this pull request May 27, 2014
…r directives, making the final implementations simpler.
@recidive
Copy link
Owner

recidive commented Sep 2, 2014

This one needs to be re-rolled. It would be nice if you could split this into smaller pull requests so changes don't hold back each other.

At least the ck-replace change should be in a different pull-request I think.

Also, I'd like to understand what are the outcomes of having everything in the main choko module instead of separate modules for directives, services, etc.?

Thanks!

recidive added a commit that referenced this pull request Oct 29, 2014
@recidive
Copy link
Owner

I've added some of this to the two commits above. I believe the only that's left is the ckReplace and ckButton refactoring.

Can this be re-rolled without renaming ckReplace directive to ckReplaceElement? I believe it makes more sense what's now ckReplace, in your changes, to be ckReplaceElement.

@recidive
Copy link
Owner

I've separate only the ckreplace refactoring changes to this branch:

https://github.com/recidive/choko/tree/ckreplace-refactoring

I would like to know what are the gains with those changes that justifies the added complexibility before mergin this upstream.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants