Skip to content

Conversation

@corneadoug
Copy link
Contributor

After so much time in the Wild Wild West of Internet, It's Time for the Spring Cleaning of Zeppelin-web.
This PR will be taking care of cleaning the code, architecture, cutting code into smaller pieces etc...

  • - Change Code Folder Structure to a Folder Tree Style
  • - Change original code and compiled code folder names
  • - Update Contributing README.md to explain most of changes
  • - Organize well the components and app folders

We will do a first merge and handle this part in a different PR:

  • - Replace as much code as possible by their lodash.js counterpart
  • - Cut the code into more smaller components (who said paragraph.js?)
  • - Move Jquery code out of the controllers (by directive when possible or to somewhere else)

Needs to make sure that:

@felixcheung
Copy link
Member

where did controllers/notebook.js or controllers/paragraph.js go?

@corneadoug
Copy link
Contributor Author

Didn't update the doc to explain the folder strcture yet.

But now, src is the source folder, and the generated code on build go to dist folder.

For the source code, you will find it organised by module (home.html, home.css, home.controller.js for example) either in the src/app folder or src/components folder.

Src/app is for page related content, and each page is containing its own modules. Src/components is for modules used in different places (navbar for example).

Now you can place files wherever you want in app and components, folders and have as much sub folder as you want, however you will need to register the .js files in index.html

@felixcheung
Copy link
Member

thanks for the explanation. I saw other files 'moved' but these two were 'deleted' as per git, so just want to check.

@corneadoug
Copy link
Contributor Author

@felixcheung looks like you were right, paragraph.js and notebook.js were not there. Realized that after git clean.

@heruka-urgyen
Copy link
Contributor

@corneadoug I skimmed through the last three commits. It looks like you did a nice job. Using controller as pattern is definitely an improvement. The other thing I have to mention that services are singletons (https://github.com/johnpapa/angular-styleguide#singletons), so you'll be better off returning an object instead of using this.
E.g.

this.deleteNotebook = function(noteId) {}
this.getNotebookList = function() {}

would become

return {
  deleteNotebook = function() {},
  getNotebookList = function() {}
}

@corneadoug
Copy link
Contributor Author

Yeah, but like your link show, services while being a singleton (just like everything in angular) are usually use as public functions only (transformation functions mainly) and do not hold any variables.

So I follow most of the examples on factory/ServiceFactory/Service differences and use this.myFunction on Services only. It kinda differentiate them

@heruka-urgyen
Copy link
Contributor

It's okay. I just have an aversion to this and try to eliminate it from my code. Up to you.

@corneadoug
Copy link
Contributor Author

This is what the Paragraph Form changes looks like.
Before, The label used to be squashed by the input, and on big screen they would not take 100% width and be under each other

screen shot 2015-06-15 at 3 52 43 pm

@heruka-urgyen
Copy link
Contributor

Looks good!

@corneadoug
Copy link
Contributor Author

I think I'm gonna Rebase master in this PR, and then it will be good to go.
This is a good base, and already a lot of work has been done here.
We can continue working on cleaning more the code in Other smaller PR.

@heruka-urgyen
Copy link
Contributor

@corneadoug go for it man!

@corneadoug
Copy link
Contributor Author

@Madhuka Not really. There is only some ng-doc on some controllers, not all (they were generated by yeoman), so it doesn't really document much. Also we don't use ng-doc in our grunt file, so its not even generating documentation out of it.

@corneadoug corneadoug force-pushed the improvement/SpringCleaning branch from 7360abb to 15cc7b1 Compare July 1, 2015 08:28
@corneadoug
Copy link
Contributor Author

Just Rebased it all, Need to verify and modify some previous pull requests now

@corneadoug
Copy link
Contributor Author

This is good for Test and Merge :)

@heruka-urgyen
Copy link
Contributor

Is it okay that Travis is complaining?

@corneadoug
Copy link
Contributor Author

I forgot to do the mvn verify. It could be the license check failing.
Anyway, I will fix it

@corneadoug
Copy link
Contributor Author

CI is green!

@heruka-urgyen
Copy link
Contributor

Looks like it's ready for merge!

@Leemoonsoo
Copy link
Member

LGTM let's merge

@Madhuka
Copy link
Contributor

Madhuka commented Jul 2, 2015

Great, Seems I can start my work on here.

On Thu, Jul 2, 2015 at 9:31 AM, Lee moon soo [email protected]
wrote:

LGTM let's merge


Reply to this email directly or view it on GitHub
#56 (comment)
.

Cheers,
Madhuka Udantha
http://madhukaudantha.blogspot.com

@asfgit asfgit closed this in 8c7424a Jul 2, 2015
This was referenced Jul 3, 2015
asfgit pushed a commit that referenced this pull request Jul 3, 2015
Ping feature to keep websocket was introduced in #109 .
But it was deleted during the merge of #56.

This is bringing it back

Author: Damien Corneau <[email protected]>

Closes #137 from corneadoug/fix/ping and squashes the following commits:

9677c30 [Damien Corneau] Remove spaces
4ccb246 [Damien Corneau] Fix ZEPPELIN-113
asfgit pushed a commit that referenced this pull request Jul 3, 2015
After #56 Interpreter creation was broken. (There was the new group.name interpreter feature to handle)
This PR handles it.

Author: Damien Corneau <[email protected]>

Closes #138 from corneadoug/fix/interpreterDropdown and squashes the following commits:

076fc2b [Damien Corneau] Fix Interpreter Group in Zeppelin-web
@corneadoug corneadoug deleted the improvement/SpringCleaning branch September 8, 2015 04:46
Leemoonsoo pushed a commit to Leemoonsoo/zeppelin that referenced this pull request Sep 17, 2015
After so much time in the Wild Wild West of Internet, It's Time for the Spring Cleaning of Zeppelin-web.
This PR will be taking care of cleaning the code, architecture, cutting code into smaller pieces etc...

* [x] - Change Code Folder Structure to a Folder Tree Style
* [x] - Change original code and compiled code folder names
* [x] - Update Contributing README.md to explain most of changes
* [x] - Organize well the components and app folders

We will do a first merge and handle this part in a different PR:
* [ ] - Replace as much code as possible by their lodash.js counterpart
* [ ] - Cut the code into more smaller components (who said paragraph.js?)
* [ ] - Move Jquery code out of the controllers (by directive when possible or to somewhere else)

Needs to make sure that:
* [x] - apache#127 is handled

Author: Damien Corneau <[email protected]>
Author: CORNEAU Damien <[email protected]>

Closes apache#56 from corneadoug/improvement/SpringCleaning and squashes the following commits:

453af1a [Damien Corneau] Merge Master and Fix ports
678c0fa [Damien Corneau] Fix RAT excluded and add Apache licenses in zeppelin-web
0addb80 [Damien Corneau] Change AppScriptServlet configuration
ef764fc [Damien Corneau] Improve uglifyjs options
15cc7b1 [CORNEAU Damien] Fix README
e3ca174 [CORNEAU Damien] Small fix in doc
775f3ca [Damien Corneau] Remove unused ngdoc comments
25a3a63 [Damien Corneau] Fix Interpreter Create form
bdde389 [Damien Corneau] Set loonknfeel to default for everypage, and change only if looknfeel is different
bdf3a8e [Damien Corneau] Include lodash + Interpreter Web refactoring Part1: reducing code
931067a [Damien Corneau] Align form label to form input + improve form disable opacity
75d12c3 [Damien Corneau] Fix CSS of paragraph forms
e3f3016 [Damien Corneau] Fix ZEPPELIN-102
a6ec901 [Damien Corneau] Fix ZEPPELIN-103
7eccca8 [Damien Corneau] Fix navbar selected menu + small code improvement
a1fe1c1 [Damien Corneau] Refactoring of Websocket
a36adf9 [Damien Corneau] Move all websocket calls to a service
b21cc69 [Damien Corneau] Refactor Navbar controller to controller pattern + data factory
5a40c4c [Damien Corneau] Separate navbar to its own html file
2dac138 [Damien Corneau] Move directives to solo directory
9201360 [Damien Corneau] Fix project after git clean
9b249ea [Damien Corneau] Clean JSHint errors except for already defined and configuration functions related errors
ef6baa0 [CORNEAU Damien] Update Zeppelin-web CONTRIBUTING.md
411df6a [Damien Corneau] Create Zeppelin-web CONTRIBUTING.md
d3c22cf [CORNEAU Damien] Update Zeppelin-web README.md
48eed51 [Damien Corneau] Move the font css
3e28c3c [Damien Corneau] Change Zeppelin-web code and compiled code folders
0ee04a2 [Damien Corneau] Fix Grunt watch
15b502c [Damien Corneau] Change Zeppelin Folder Structure and its GruntFile
9f9059e [Damien Corneau] Add spark dependency reduced pom to gitignore

(cherry picked from commit 8c7424a)
Signed-off-by: Lee moon soo <[email protected]>
Leemoonsoo pushed a commit to Leemoonsoo/zeppelin that referenced this pull request Sep 17, 2015
Ping feature to keep websocket was introduced in apache#109 .
But it was deleted during the merge of apache#56.

This is bringing it back

Author: Damien Corneau <[email protected]>

Closes apache#137 from corneadoug/fix/ping and squashes the following commits:

9677c30 [Damien Corneau] Remove spaces
4ccb246 [Damien Corneau] Fix ZEPPELIN-113

(cherry picked from commit 94c7375)
Signed-off-by: Lee moon soo <[email protected]>
Leemoonsoo pushed a commit to Leemoonsoo/zeppelin that referenced this pull request Sep 17, 2015
After apache#56 Interpreter creation was broken. (There was the new group.name interpreter feature to handle)
This PR handles it.

Author: Damien Corneau <[email protected]>

Closes apache#138 from corneadoug/fix/interpreterDropdown and squashes the following commits:

076fc2b [Damien Corneau] Fix Interpreter Group in Zeppelin-web

(cherry picked from commit 3f87f44)
Signed-off-by: Lee moon soo <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants