showstopping js bugs removed. Other changes... #10

Merged
merged 1 commit into from Jul 8, 2012

Conversation

Projects
None yet
2 participants
@sunilw

sunilw commented Jul 7, 2012

Major code alterations. 'setgridonload' has been removed. I've added a
new object 'hugrid', and attached it to 'window'. This was an excercise
is keeping things clean and tidy. Moved 'gridstate' into our new
object.
I've also tried to satisfy jshint as much as I could understand it.

New object added to window. The new object has a new implimentation of 'gridstate' and methods for checking and toggling it.

This fork no longer has the bugs that stop js execution. It passes my own testing, so it's ready to be tested further by someone who isn't me.

Let me know what you think.

Thanks.

Sunil Williams
Remove showstopping scope bugs.
Major code alterations. 'setgridonload' has been removed. I've added a
new object 'hugrid', and attached it to 'window'. This was an excercise
is keeping things clean and tidy. Moved 'gridstate' into our new
object.
I've also tried to satisfy jshint as much as I could understand it.
@simanek

This comment has been minimized.

Show comment
Hide comment
@simanek

simanek Jul 7, 2012

Owner

Looks good. I'll need to test it yet. Here's how I understand the changes, let me know if I'm misunderstanding:

  • Now these hugrid elements are all children of an object rather than being... I guess direct children of the document object. Sounds like some advice I've heard, but didn't know how to do.
  • Rather than creating the objects and then changing their visibility with CSS by adding and removing classes from the elements, now we are simply removing and adding the elements. I'm a bit uncomfortable with that since I have a solid understanding of CSS but if it works the same, great.
  • You've removed the 'gridonload' variable, but it is needed. This is how the users can define whether or not the grid is initially on or off on pageload. I'll try to decipher this new code enough to reintroduce that functionality without reintroducing the script errors.
  • The new line at the end wasn't there? I thought I had it there, but maybe my text editor is doing something funny with empty newlines at the end of documents.

Thanks for this work!

Owner

simanek commented Jul 7, 2012

Looks good. I'll need to test it yet. Here's how I understand the changes, let me know if I'm misunderstanding:

  • Now these hugrid elements are all children of an object rather than being... I guess direct children of the document object. Sounds like some advice I've heard, but didn't know how to do.
  • Rather than creating the objects and then changing their visibility with CSS by adding and removing classes from the elements, now we are simply removing and adding the elements. I'm a bit uncomfortable with that since I have a solid understanding of CSS but if it works the same, great.
  • You've removed the 'gridonload' variable, but it is needed. This is how the users can define whether or not the grid is initially on or off on pageload. I'll try to decipher this new code enough to reintroduce that functionality without reintroducing the script errors.
  • The new line at the end wasn't there? I thought I had it there, but maybe my text editor is doing something funny with empty newlines at the end of documents.

Thanks for this work!

@simanek

This comment has been minimized.

Show comment
Hide comment
@simanek

simanek Jul 7, 2012

Owner

Also, what's the difference between != and !== ?

Owner

simanek commented Jul 7, 2012

Also, what's the difference between != and !== ?

@sunilw

This comment has been minimized.

Show comment
Hide comment
@sunilw

sunilw Jul 8, 2012

Thanks.

The gridonload variable is still there. It's just tucked away, and not being used by anything. It's accessible as 'window.hugrid.state.gridonload'.

I did however, remove the setgridonload function (which uses it) because I couldn't figure out what it was doing. As far as I could tell, it seemed to be generating errors without being meaningful.

So, if I understand correctly, that function is supposed to enable the grids visibility as a default upon the page load. Is that correct?

Currently, the grid is visible whether that function exists or not. As far as I could tell, setgridonload never executed, because the 'if' statement would always error out.

So, if you adjudge my understanding to be correct, here is my suggestion: allow the function to be absent for the moment. And then create a functionining way to toggle the grids visibility on page load.

"Also, what's the difference between != and !== ?"

I'm really not sure. The use of the former over the latter yielded whinging from jshint. I made the changes and tested. The results seemed to be acceptable.

What is the significance of the extra newline?

sunilw commented Jul 8, 2012

Thanks.

The gridonload variable is still there. It's just tucked away, and not being used by anything. It's accessible as 'window.hugrid.state.gridonload'.

I did however, remove the setgridonload function (which uses it) because I couldn't figure out what it was doing. As far as I could tell, it seemed to be generating errors without being meaningful.

So, if I understand correctly, that function is supposed to enable the grids visibility as a default upon the page load. Is that correct?

Currently, the grid is visible whether that function exists or not. As far as I could tell, setgridonload never executed, because the 'if' statement would always error out.

So, if you adjudge my understanding to be correct, here is my suggestion: allow the function to be absent for the moment. And then create a functionining way to toggle the grids visibility on page load.

"Also, what's the difference between != and !== ?"

I'm really not sure. The use of the former over the latter yielded whinging from jshint. I made the changes and tested. The results seemed to be acceptable.

What is the significance of the extra newline?

@simanek

This comment has been minimized.

Show comment
Hide comment
@simanek

simanek Jul 8, 2012

Owner

Obviously you have pointed out that my script as it was written was throwing several errors, but the setgridonload function was part of a working feature. I'm not sure why it didn't appear to execute for you. Regardless, I'll figure out how to make it work within this improved structure that you've created.

As for the "not equal to", seems like one or two equal signs is basically the difference of "not equal to" and "not equal to and not of the same type"... http://www.devguru.com/technologies/ecmascript/quickref/comparison_operators.html
Why one or the other is correct for this script, I can't discern, but as long as it works.

The newline bit? I was just responding to the diff changes. I'm trying to make sure I understand your changes. I need to learn more about JavaScript. :D

Owner

simanek commented Jul 8, 2012

Obviously you have pointed out that my script as it was written was throwing several errors, but the setgridonload function was part of a working feature. I'm not sure why it didn't appear to execute for you. Regardless, I'll figure out how to make it work within this improved structure that you've created.

As for the "not equal to", seems like one or two equal signs is basically the difference of "not equal to" and "not equal to and not of the same type"... http://www.devguru.com/technologies/ecmascript/quickref/comparison_operators.html
Why one or the other is correct for this script, I can't discern, but as long as it works.

The newline bit? I was just responding to the diff changes. I'm trying to make sure I understand your changes. I need to learn more about JavaScript. :D

@simanek

This comment has been minimized.

Show comment
Hide comment
@simanek

simanek Jul 8, 2012

Owner

I'm working on this now. The script certainly works, but the "gridstate" and "gridonload" features are broken.

gridstate is always defaulting to "on" (which is kind of what I expected after reading your code changes – I'm learning to read JS! :D)

I created the gridstate variable to preserve the on/off state of the grid as the browser window is resized. Since the grid is actually recreated every time the window size is adjusted, gridstate would keep the grid on or off as it was previously set by the user prior to adjusting the window size.

And as described previously, gridonload is a way for the website developer to define whether the grid is on or off on the initial pageload.

I'll see what I can do.

Owner

simanek commented Jul 8, 2012

I'm working on this now. The script certainly works, but the "gridstate" and "gridonload" features are broken.

gridstate is always defaulting to "on" (which is kind of what I expected after reading your code changes – I'm learning to read JS! :D)

I created the gridstate variable to preserve the on/off state of the grid as the browser window is resized. Since the grid is actually recreated every time the window size is adjusted, gridstate would keep the grid on or off as it was previously set by the user prior to adjusting the window size.

And as described previously, gridonload is a way for the website developer to define whether the grid is on or off on the initial pageload.

I'll see what I can do.

@simanek

This comment has been minimized.

Show comment
Hide comment
@simanek

simanek Jul 8, 2012

Owner

OK. I've been able to take your changes and successfully reintroduce the gridonload and gridstate features!

Owner

simanek commented Jul 8, 2012

OK. I've been able to take your changes and successfully reintroduce the gridonload and gridstate features!

simanek added a commit that referenced this pull request Jul 8, 2012

Merge pull request #10 from sunilw/master
showstopping js bugs removed. Other changes...

@simanek simanek merged commit 70f6bfa into simanek:master Jul 8, 2012

@simanek

This comment has been minimized.

Show comment
Hide comment
@simanek

simanek Jul 8, 2012

Owner

If you could, please take a look at my commit. I'm sure I didn't do it in the best possible way, but it IS working and your fixes for the JS errors still seem to work. Hopefully my sloppy coding will at the very least make my intent apparent. Let me know if you have any ideas about how to improve it further.

Owner

simanek commented Jul 8, 2012

If you could, please take a look at my commit. I'm sure I didn't do it in the best possible way, but it IS working and your fixes for the JS errors still seem to work. Hopefully my sloppy coding will at the very least make my intent apparent. Let me know if you have any ideas about how to improve it further.

@sunilw

This comment has been minimized.

Show comment
Hide comment
@sunilw

sunilw Jul 8, 2012

All good Jason. I'm pleased to be able to help.

I've briefly assessed the update. It looks good and seems to function as intended.

sunilw commented Jul 8, 2012

All good Jason. I'm pleased to be able to help.

I've briefly assessed the update. It looks good and seems to function as intended.

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