fixed appirio-tech/topcoder-app/issues/1040 refactored details page b…

[shubhendusaurabh]

fixed appirio-tech/topcoder-app/issues/1040 refactored details page btons

[birdofpreyru]

Hey, so far I've just briefly looked at the code, and I might misunderstood something (don't hesitate to argue and correct me), but here are the changes, I believe you should make:

1. Get rid of the getButton() function in its current form. I believe, now each time you call this function, it first creates a new buttons object, and then returns the specified button from that object - unefficient. I'd better have all properties of each button specified right inside vm.puttons.push() argument, probably passing them through a newButton() method, to verify the correct use. I.e. smth like:

   vm.buttons.push(newButton({
   href: '...',
   text: '...'
   }))

   where newButton() will (1) document which fields an object button may have, (2) validates that you pass a correct set of params, i.e. it does not make sense to pass both href and onClick, though you should pass in one of them.

2. Let's get rid of spanText. Instead, in the span we can show just the index of button inside vm.buttons array, shifted by one. So that if your condition say a button should not be included in the array, the remaining buttons will automatically have proper numbers on them, depending on their order inside the array.

3. Any reason to pass onClick as text? Why don't we pass the handlers by function reference? - I'd prefer this way.

[shubhendusaurabh]

@birdofpreyru Updated

good code review :+1:

[birdofpreyru]

Good, but contains some errors in the logic.

1. Registration button is inactive for challenges with open registration.

2. If I register for a challenge using production website, and then click Unregister in the local deployment, running your changes, here is what I see (buttons array is not reset at some point):

[shubhendusaurabh]

@birdofpreyru fixed