-
Notifications
You must be signed in to change notification settings - Fork 93
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
[BROOKLYN-166] add codemirror, yaml syntax-highlighting, autocompletion) #865
base: master
Are you sure you want to change the base?
Conversation
Testing from Tested with org.apache.brooklyn.rest.jsgui.BrooklynJavascriptGuiLauncher -agentlib:jdwp=transport=dt_socket,server=y,suspend=n,address=5005 I'll take a look at what's missing, tomorrow |
incubator-brooklyn-pull-requests #1728 SUCCESS |
Tested, it works great. There are indeed two scrolls but that should be easily fixable with a bit of CSS. Also, the editor is only one line height, could we extend to the height of the pop-up by default? Regarding the autocompletion, my view on that is first to pull out the catalogue entities when we are on a |
@tbouron, regarding extending to the hight of the pop-up, we could integrate the tree view you proposed a while back. I don't know if that could be done without enlarging the pop-up which will impact the other tabs. Unless we make it a different pop-up or view. WDYT? |
A few comments:
Of course we need RAT re-enabled. Code completion will require some REST API enhancements or better understanding of the model in the client. I'd also like to see this as a separate main tab, rather than a wizard. But we can leave these for a follow-on PR, as with the bug fixes before this paragraph resolved, this is a nice improvement. |
There is a Brooklyn camp yaml reference on the website. Most useful thin
Cloudsoft Corporation Limited, Registered in Scotland No: SC349230. This e-mail message is confidential and for use by the addressee only. If Whilst all reasonable care has been taken to avoid the transmission of |
I think it's this page, right? https://brooklyn.incubator.apache.org/v/latest/yaml/yaml-reference.html |
@hzbarcea To be honest, I'm not so sure anymore about the drag and drop editor. It is certainly a good feature for a sales point of view but I'm wondering about the usability. I think that having a good YAML editor with a good autocompletion feature over Brooklyn CAMP spec is a better approach for now. We could also imagine to have something a bit like swagger, i.e. having a live representation of the YAML as a tree or nodes. That would be great! |
I think a drag-and-drop editor is very important for new users, especially if it generates the YAML which will help them move to become advanced users. |
@azbarcea can you address the issues blocking this being merged? |
ping @azbarcea |
incubator-brooklyn-pull-requests #1815 FAILURE |
72a9ebf
to
b42ebdb
Compare
fixed merge ... with latest code base |
incubator-brooklyn-pull-requests #1816 SUCCESS |
@@ -25,7 +25,7 @@ BODY { | |||
textarea { | |||
white-space: pre; | |||
word-wrap: normal; | |||
overflow-x: scroll; | |||
/* overflow-x: scroll; */ |
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.
This line can be removed
I fixed the indentation ... any thoughts? |
@azbarcea +1. maybe call it "Composer" rather than "Editor", and I think don't show the catalog on this tab (we have the catalog tab). |
@ahgittin I'll rename it to composer! +1 Regarding the Catalog, I think is a good proposal (+1), and I can have propose it as a different PR, based also on the community feedback. I'm working to resolve the Jenkins build also ... I'll do the merge! |
Conflicts: - trivial - license inclusion updates nearby, and apidoc removed where editordoc added usage/jsgui/src/main/license/files/LICENSE usage/jsgui/src/main/license/source-inclusions.yaml usage/jsgui/src/main/webapp/assets/js/router.js
merge conflicts with recent swagger changes but i've resolved those; now reviewing |
a lot of |
@@ -26,6 +26,11 @@ | |||
|
|||
<title>Brooklyn JS REST client</title> | |||
|
|||
<!-- TODO: following CSS to config.js --> |
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.
what does this mean?
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.
this line means that the CSS from index.html should be minimized as well ... I will address this issue!
I will remove the logs. They had some meaning for me, but definitely doesn't make sense for everybody. The logs should be removed though by the minification
step.
the following issues are still outstanding:
new ones:
ability to test is limited by the above issues! |
@@ -18,14 +18,15 @@ | |||
*/ | |||
define([ | |||
"brooklyn", "underscore", "jquery", "backbone", | |||
"codemirror", |
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.
this is what is breaking the run -- we don't use it locally so we can remove it I think; but if it's added of course there needs to be a corresponding CodeMirror
argument to the following function
i've fixed the problem with startup. now looks much better, and tests work. however:
|
my fix for the @azbarcea I suggest you merge it in here |
merged with @ahgittin |
…irror Continuatiion of work started in apache/incubator-brooklyn#865 for BROOKLYN-166
…irror Continuatiion of work started in apache/incubator-brooklyn#865 for BROOKLYN-166
I was looking into the
codemirror
integration.I didn't integrate it in all the views, only in
application-add-wizard.js
, and I look forward to your feedback.I tried to improve the
scroll down
and now, there aren't 2 scrollers. There is only one for the editor.The following things are still open:
rat-plugin
for the files added (actually, right now excludes the wholesrc
).codemirror
instead of../../lib/codemirror
angular.js
views andrequire.js
is used (using theconfig.js
), make the presumptions thejavascript
libraries are static inlibs
. Similar tomvn
, I proposegrunt
andnpm
to be used, so that the wholelibs
directory to disappear.yaml-hint.js
, or evencamp-hint.js
. I need to study further the CAMP spec for it. I think also thatbrooklyn
has a little different spec for.yaml
files, extending the language. I am right? Right now, I used theanyword-hint.js
, which adds to the hints (auto-completion), the words that already exist in the document.