Skip to main content
Code Review

Return to Answer

replaced http://stackoverflow.com/ with https://stackoverflow.com/
Source Link

I would not hard code 'container', instead make it an attribute of gridOptions :

var options = { 
 ....
 containerClass = 'container',
 ....
}

I would consider extending the options to include an id or selector of a parent (an anchor) where to add the grid (if null then use document.body as you do now).You could also add to the options the ID of the grid being created, then you would be potentially be able to handle multiple grids in the same document).

Every time the window is resized the grid is recreated, meaning that the keyup listener is attached to the document multiple times, and similarly changeGridOptions() is called every time the user presses the enter key, resulting in multiple calls to changeButton.addEventListener().

All these listeners should be removed (see http://stackoverflow.com/questions/14040386/should-i-remove-event-handlers-from-element-before-removechild https://stackoverflow.com/questions/14040386/should-i-remove-event-handlers-from-element-before-removechild) when the grid is cleared , ie when a new grid is created, by calling window.removeEventListener (See http://stackoverflow.com/questions/2991382/how-do-i-add-and-remove-an-event-listener-using-a-function-with-parameters https://stackoverflow.com/questions/2991382/how-do-i-add-and-remove-an-event-listener-using-a-function-with-parameters)

There are 2 things that should happen when clearing the grid:

  1. the timeout should be cleared (only done in onresize())
  2. the container DOM element must be removed (implemented in 2 different places: the timer and on resize)

This duplication and inconsistency can be avoided by moving this code to the constructor of the grid: check if the container is already created (by class or by id if you provide one as I suggest above) and first clear the interval if it is set, then remove any registered listeners, then remove the DOM element, and then instantiate the Grid.

I would not hard code 'container', instead make it an attribute of gridOptions :

var options = { 
 ....
 containerClass = 'container',
 ....
}

I would consider extending the options to include an id or selector of a parent (an anchor) where to add the grid (if null then use document.body as you do now).You could also add to the options the ID of the grid being created, then you would be potentially be able to handle multiple grids in the same document).

Every time the window is resized the grid is recreated, meaning that the keyup listener is attached to the document multiple times, and similarly changeGridOptions() is called every time the user presses the enter key, resulting in multiple calls to changeButton.addEventListener().

All these listeners should be removed (see http://stackoverflow.com/questions/14040386/should-i-remove-event-handlers-from-element-before-removechild) when the grid is cleared , ie when a new grid is created, by calling window.removeEventListener (See http://stackoverflow.com/questions/2991382/how-do-i-add-and-remove-an-event-listener-using-a-function-with-parameters)

There are 2 things that should happen when clearing the grid:

  1. the timeout should be cleared (only done in onresize())
  2. the container DOM element must be removed (implemented in 2 different places: the timer and on resize)

This duplication and inconsistency can be avoided by moving this code to the constructor of the grid: check if the container is already created (by class or by id if you provide one as I suggest above) and first clear the interval if it is set, then remove any registered listeners, then remove the DOM element, and then instantiate the Grid.

I would not hard code 'container', instead make it an attribute of gridOptions :

var options = { 
 ....
 containerClass = 'container',
 ....
}

I would consider extending the options to include an id or selector of a parent (an anchor) where to add the grid (if null then use document.body as you do now).You could also add to the options the ID of the grid being created, then you would be potentially be able to handle multiple grids in the same document).

Every time the window is resized the grid is recreated, meaning that the keyup listener is attached to the document multiple times, and similarly changeGridOptions() is called every time the user presses the enter key, resulting in multiple calls to changeButton.addEventListener().

All these listeners should be removed (see https://stackoverflow.com/questions/14040386/should-i-remove-event-handlers-from-element-before-removechild) when the grid is cleared , ie when a new grid is created, by calling window.removeEventListener (See https://stackoverflow.com/questions/2991382/how-do-i-add-and-remove-an-event-listener-using-a-function-with-parameters)

There are 2 things that should happen when clearing the grid:

  1. the timeout should be cleared (only done in onresize())
  2. the container DOM element must be removed (implemented in 2 different places: the timer and on resize)

This duplication and inconsistency can be avoided by moving this code to the constructor of the grid: check if the container is already created (by class or by id if you provide one as I suggest above) and first clear the interval if it is set, then remove any registered listeners, then remove the DOM element, and then instantiate the Grid.

Source Link

I would not hard code 'container', instead make it an attribute of gridOptions :

var options = { 
 ....
 containerClass = 'container',
 ....
}

I would consider extending the options to include an id or selector of a parent (an anchor) where to add the grid (if null then use document.body as you do now).You could also add to the options the ID of the grid being created, then you would be potentially be able to handle multiple grids in the same document).

Every time the window is resized the grid is recreated, meaning that the keyup listener is attached to the document multiple times, and similarly changeGridOptions() is called every time the user presses the enter key, resulting in multiple calls to changeButton.addEventListener().

All these listeners should be removed (see http://stackoverflow.com/questions/14040386/should-i-remove-event-handlers-from-element-before-removechild) when the grid is cleared , ie when a new grid is created, by calling window.removeEventListener (See http://stackoverflow.com/questions/2991382/how-do-i-add-and-remove-an-event-listener-using-a-function-with-parameters)

There are 2 things that should happen when clearing the grid:

  1. the timeout should be cleared (only done in onresize())
  2. the container DOM element must be removed (implemented in 2 different places: the timer and on resize)

This duplication and inconsistency can be avoided by moving this code to the constructor of the grid: check if the container is already created (by class or by id if you provide one as I suggest above) and first clear the interval if it is set, then remove any registered listeners, then remove the DOM element, and then instantiate the Grid.

default

AltStyle によって変換されたページ (->オリジナル) /