search

prahladyeri / VisualAlchemist

Open source database diagramming and automation tool
GNU General Public License v3.0
728 stars 57 forks source link

Fixed Details column comma, improved error messages, fixed canvas zoom in/out #32

Closed calebmauer closed 8 years ago

calebmauer commented 8 years ago

Fixed commas in details column to not have double commas or having a comma when there is only one thing in the details row. Edited error messages to be more descriptive. Changed error messages to use bspopup instead of alert. Moved embedded css into app.css. Shortened and cleaned up code in app.js addTable section.

prahladyeri commented 8 years ago

Hey @calebmauer

Thanks a lot for refactoring this code, I'd been meaning to do this myself since a while but was unable to take the time to do so. I just have a few queries regarding the changes you made:

  1. Why did you remove document.getElementById("fieldName").focus(); (on line 56 of assets/partials/addTableDialog.html) ?
  2. Similarly, the style tag on line 217 ?
  3. I see that you have added height: 100vh; to body and similar attributes to the #theCanvas object in app.css. What does this do (I'm not that great at CSS myself).

Finally, I hope you have tested the app thoroughly before submitting this PR?

calebmauer commented 8 years ago
  1. I tested it and that focus() call was redundant. The same thing is being done elsewhere. I tested it and the text box is always getting focused when it's supposed to so this line is OK to remove.
  2. I moved that style tag into the app.css file. This is a best practice with css - always put css in a css file unless you have no other option, and this was an easy case of being able to put it in a css file.
  3. The height 100vh makes the canvas take up the whole screen. This makes it so the canvas is always filling the screen even when you zoom out. I don't know about you, but on my high resolution monitor I was seeing a big whitespace at the bottom of the screen that the canvas didn't cover.

Yep, I tested this all thoroughly. :)

On Sat, Jun 18, 2016 at 9:01 PM, Prahlad Yeri notifications@github.com wrote:

Hey @calebmauer https://github.com/calebmauer

Thanks a lot for refactoring this code, I'd been meaning to do this myself since a while but was unable to take the time to do so. I just have a few queries regarding the changes you made:

  1. Why did you remove document.getElementById("fieldName").focus(); (on line 56 of assets/partials/addTableDialog.html) ?
  2. Similarly, the style tag on line 217 ?
  3. I see that you have added height: 100vh; to body and similar attributes to the #theCanvas object in app.css. What does this do (I'm not that great at CSS myself).

Finally, I hope you have tested the app thoroughly before submitting this PR?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/prahladyeri/VisualAlchemist/pull/32#issuecomment-226973437, or mute the thread https://github.com/notifications/unsubscribe/ABc2-4wdgBb3_E76XNbOu2TJG-dP55gnks5qNJTlgaJpZM4I380u .

prahladyeri commented 8 years ago

@calebmauer Thanks for your contribution. Please note that as mentioned in contributor guidelines, this is a GPLv3 project, so please make sure that all your contributed code complies accordingly.

calebmauer commented 8 years ago

I'm pretty sure they do. The only code I don't come up with on my own is maybe stuff from Stack Overflow but if I copy code directly from Stack Overflow I will add a comment with the URL of the post I got it from.

On Sat, Jun 18, 2016 at 10:15 PM, Prahlad Yeri notifications@github.com wrote:

@calebmauer https://github.com/calebmauer Thanks for your contribution. Please note that as mentioned in contributor guidelines, this is a GPLv3 project, so please make sure that all your contributed code complies accordingly.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/prahladyeri/VisualAlchemist/pull/32#issuecomment-226975376, or mute the thread https://github.com/notifications/unsubscribe/ABc2--w-o4S8oZ9jEAwnpVysHxLnCKJOks5qNKYzgaJpZM4I380u .

prahladyeri commented 8 years ago

Perfect!