Closed gkamal closed 11 years ago
gkamal
commented
11 years ago Hi,
I fixed the error, moved the core logic into drawingengine.js and added a few tests. Please take a look.
I couldn't find an easy way to fix the popup. One solution is to use a popup dialog from jquery-ui. Should I try that?
ogt
commented
11 years ago I couldn't find an easy way to fix the popup. One solution is to use a popup dialog from jquery-ui. Should I try that? I see. I was afraid that this was the case. Lets leave it the way you have it.
..and added a few tests. Almost perfect! I would like though to maintain the block coverage at 100%
- it dropped to 96% now. https://github.com/ogt/boxchareditor/blob/gh-pages/README.md#contributions
Do you mind adding one test that tests the invalid bounds code - its the only un-convered line.
Just do that and we are done!
odysseas
On Tue, May 21, 2013 at 7:22 AM, Kamal Govindraj notifications@github.comwrote:
Hi,
I fixed the error, moved the core logic into drawingengine.js and added a few tests. Please take a look.
I couldn't find an easy way to fix the popup. One solution is to use a popup dialog from jquery-ui. Should I try that?
— Reply to this email directly or view it on GitHubhttps://github.com/ogt/boxchareditor/pull/19#issuecomment-18211838 .
Hi @gkamal, Great job ! A few points. The first one I think is a bug... the rest are improvement suggestions, ie optional.
A. It seems that you are not copying the last row: See this example:
which reults in copying this area:
B. (optional) The popup show the text a displaying a single line. Can you make it display the full selected area?
C. (optional) You included all the code in index.html. It would be better if you pushed some of the functionality into the drawingengine.js and added a test against it. (much harder to add automated tests against index.html. The example I gave you can be all that you test, given an input grid , and from - to points, to check and make sure that the select_area function returns you the right buffer.