-
Notifications
You must be signed in to change notification settings - Fork 1.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Ace editor & Model Job UI Tweaks #751
Conversation
Very cool, great addition! I've tried it and it works great so far, let me start reviewing the code...
|
Sure Luke. Note the CLA link is dead in https://github.com/NVIDIA/DIGITS/blob/master/.github/CONTRIBUTING.md#L33 |
Whoops, good call. #753. |
@@ -376,14 +382,16 @@ <h4 style="display:inline-block;">Python Layers</h4> | |||
) | |||
.done(function(data) { | |||
data = $.parseJSON(data); | |||
$('textarea[name=custom_network]').val(data['network']); | |||
editor.setValue(data['network']); | |||
editor.setValue(editor.getValue(), 1); // Moves cursor to end of editor |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we move the cursor to the top instead of the end? Did you have a particular reason for wanting to see the end first?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry Luke, I insist we keep it at the bottom. Do you have a particular reason for wanting to see the start first?
No, seriously; I couldn't care less. I did it because by default it selects all the text by default when set text is Set(). Therefore i just put the cursor at the end: for torch this is often the most convenient as the top is often full of inclusions and stuff I rarely touch.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Opening a text document at the tail is nonstandard. Selecting all the text is really nonstandard. I suggest having it not select any text, and open at the head for this PR, and if you prefer, change it to open at the tail in your repo.
For reference, here are some alternate themes available for Ace: I don't have a strong opinion on the theme - "Monokai" works for me. @jmancewicz: do you have an opinion? |
<a href=# class="btn btn-info" onClick="return visualizeNetwork();">Visualize</a> | ||
</div> | ||
{{form.custom_network(class='form-control', rows=10)}} | ||
<div id="divCheckbox" style="display: none;">{{form.custom_network(class='form-control', rows=10)}}</div> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was going to suggest that divCheckbox
be div_checkbox
or div-checkbox
as long as it was new, but then saw that it wasn't being used. Is there a reason to id that div?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah that's a shameful copy-paste error. Well spotted.
Based on their license, can you add these lines to the top of those minimized JS files? // Copyright (c) 2010, Ajax.org B.V.
// All rights reserved. |
All set and ready for review. |
Technically you didn't do this one, but who cares. I still need your CLA before I can merge, but otherwise this looks great! I like the Chrome theme, it seems to fit in pretty well. |
CLA is on the other computer, keep forgetting to send it. Actually i did do the id removal but only for the generic and not the classification. In fact i encounter those minor 'mistakes' more often wrt the classification/generic files. |
I know, it's a pain. Fortunately @gheinrich is helping us forge ahead towards a nice, general solution that will let us de-duplicate a lot of that code eventually. |
Fixed. |
CLA received. Looks great. Thanks for the contribution! |
Ace editor & Model Job UI Tweaks
Implements #738.
Note that for purposes of backwards compatibility and maintaining flask/python integrity, the custom_network container is still in place, and only syncs with the ace editor when it has to. Although the file diffs seem to indicate i did a lot, the ace editor is only four .js files and a few LOC, while the majority of the code change is just moving around the
<div>
's to tweak the model creation UI.