concord-consortium / lab-interactives-site

Set of interactives built using the Lab Framework from the Concord Consortium
lab.concord.org
Other
14 stars 28 forks source link

Final version dissociation-energy2 #31

Closed bhavna19nagpal closed 9 years ago

ddamelin commented 9 years ago

Functionally this seems good. There is one change you could make that would improve the layout. Change the slider width from "100%" to "auto". Then I'll merge this and post it for the team to look at.

ddamelin commented 9 years ago

Oh. And there was one other thing. You have a dissociationEnergy parameter that you don't need here. You should move the setBondEnergy call to an onLoad section of the models section of the interactive definition. Look for an example in the other interactives about how to set up the onload section.

ddamelin commented 9 years ago

Did you try to run this locally? It doesn't seem like it would work given the way you wrote the setBondEnergy call. It should have the form and value similar to what you had before when it was being set as a parameter linked to a slider. That method takes two arguments, the element pair and the bond strength.

scytacki commented 9 years ago

Can you rebase this work so it doesn't include the merge commits?

Also it seems something isn't setup right with the timezone on your computer. Github is listing the commits out of order (at least for me). Looking at the parent commit of each one shows me the right order, but I think github lists them by time instead of ordering them like a linked list.

bhavna19nagpal commented 9 years ago

Sir, This code has been fixed, to my knowledge. The method you mentioned was giving an error in the "error solved for onLoad" merge, when put into onLoad section of the models.

"bondEnergy": {
            "default": 10,      
            "1-1": 5        
          },
bhavna19nagpal commented 9 years ago

Sir, I have rebased all the work, and I have edited the code for bond energy , as well.

"bondEnergy" {
        "default": 4,       
        "1-1": 3        
       },  
ddamelin commented 9 years ago

Bahvna,

This change is not in the right direction. You were on the right track with the previous work for setBondEnergy, but you didn't make a proper call to that function. You need to look at how this function is typically called in other examples, or even the original example of this code. This commit you pushed to your branch doesn't build properly either. It is crucial that you get your changes running locally on your machine without errors before you push any code to github.

bhavna19nagpal commented 9 years ago

Sir, Do you mean I just need to set

"setBondEnergy('1-1',4 );"

Also, I will take care of code that I upload from next time.