js: users module refactoring #1233

vitoravelino · 2017-03-04T20:39:33Z

So, this is something I've been thinking about for a while since I started contributing to Portus. After #1238 were merged, we are now able to write js using the new ES2015 syntax. This will improve the code readability and maintainability.

One of the coolest feature on ES2015 is the module support. The folder structure I'm proposing is something similar to:

javascripts/
    modules/
        (...)      
        namespaces/
        teams/
            components/
                form.js
                list.js
            pages/
                show.js
                list.js
            index.js
    application.js
modules

The index.js in the teamsfolder is simple and will be responsiable to know which which page component should be used on the loaded page. Example:

import TeamsShow from './pages/show';
import TeamsIndex from './pages/index';

// we can move this to a separated file 
// and import as `import { TEAMS_SHOW...
const TEAMS_SHOW_ROUTE = 'teams/show';
const TEAMS_INDEX_ROUTE = 'teams/index';

$(() => {
  const $body = $('body');
  const route = $body.data('route');

  if (route === TEAMS_SHOW_ROUTE) {
    TeamShow.render($body);
  }

  if (route === TEAMS_INDEX_ROUTE) {
    TeamIndex.render($body);
  }

  (...)

I know that it's quite a poor technique but since we have the module system on our side, it guarantees that only the code involved on the page component will be executed.

A BaseComponent was created as an artifact to avoid code duplication and create some convention.

Suggestions are more than welcome. 😃

vitoravelino · 2017-03-05T01:28:28Z

Tests are not passing because coverage got reduced. Any chance that we could expose the coverage report on coveralls or anything similar?

I have problem testing portusctl and couldn't reproduce the same coverage problem.

vitoravelino · 2017-03-05T15:02:42Z

app/assets/javascripts/auth/cover.js

-
-  if ($('body section.sign-up, body section.login').length) {
-    rndNum = Math.floor((Math.random() * 2) + 1);
-    $('body').addClass('massive-background-' + rndNum).hide().fadeIn(1000);


Any idea what this massive-background-n is/was used for? I didn't find anything.

vitoravelino · 2017-03-05T15:44:28Z

Most of the work is done. I'd appreciate some feedback.

mssola · 2017-03-06T11:39:33Z

@vitoravelino I'll review this PR later, but in principle it looks like a nice idea. In the meantime take a look at #1238. This is still WIP and it's something I've done during the weekend, and I think that it clashes a bit with your PR (not too much). My idea would be to get that one merged before yours, because the importing of modules will change (and also note that using webpack allows us to ignore the dependency on the execjs gem). After that we can review this one.

vitoravelino · 2017-03-06T12:10:49Z

@mssola Oh, that looks great. I didn't consider webpack because I thought it would add another thing to the build process and that would be an issue.

I'll take a look at your work. As soon as you merge your PR I'll rebase my branch and you can review it. Feeling excited for both changes. Woohooo! 😄

mssola · 2017-03-09T16:08:15Z

@vitoravelino the PRs regarding webpack and ruby 2.4 have been merged. You can go ahead 👍

vitoravelino · 2017-03-10T01:13:36Z

@mssola I've rebased and fixed the issues but there are some feature tests failing in travis but passing on localhost. It doesn't make sense. Any idea what could be? =/

vitoravelino · 2017-03-10T10:34:55Z

Enabled js_errors and I got a clue. Working on it.

vitoravelino · 2017-03-11T00:57:28Z

Finally figured out the problem. For some unknown (at least for me) reason this.onKeyup.bind(this) was throwing an error on travis but working in localhost. First, I tried to change it to use $.proxy(this.onKeyup, this)and it worked. Since we have ES2015 syntax support, I preferred to use e => this.onKeyup(e) instead because it's cleaner.

I know that the project is moving forward to use a library to manage the view layer but the structure is not gonna change much from what I've written here. 😃

mssola

In general it looks great to me. Just fix my comments (the documentation comment applies to all classes btw, I didn't want to repeat myself after some point :D) 😉

mssola · 2017-03-13T16:43:50Z

app/assets/javascripts/modules/users/components/application-token-panel.js

+const APP_TOKEN_FORM = '#add_application_token_form';
+const APP_TOKEN_FIELD = '#application_token_application';
+
+class ApplicationTokenPanel extends BaseComponent {


Could you add documentation ?

Is there any format/rule that I should follow?

mssola · 2017-03-13T16:44:06Z

app/assets/javascripts/modules/users/components/password-form.js

+const NEW_CONFIRMATION_PASSWORD_FIELD = '#user_password_confirmation';
+const SUBMIT_BUTTON = 'input[type=submit]';
+
+class UsersPasswordForm extends BaseComponent {


mssola · 2017-03-13T16:44:22Z

app/assets/javascripts/modules/users/components/profile-form.js

+const DISPLAY_NAME_FIELD = '#user_display_name';
+const SUBMIT_BUTTON = 'input[type=submit]';
+
+class UsersProfileForm extends BaseComponent {


mssola · 2017-03-13T16:46:10Z

app/views/devise/registrations/edit.html.slim

-          .form-group
-            .col-md-offset-2.col-md-7
-              = f.submit('Create', class: 'btn btn-primary')
+    .app-token-wrapper


Instead of wrapping the form, why don't you simply add this class into the form itself ?

The idea is to have an element that contains both the link and the form. I know I could just add classes/ids to the elements and access from the global scope but restricting the component to access only the this.$el give us more control of what's going on and also a good practice.

Ideally, panel and form could be separated components and when user clicked on the link an event is triggered and the component that contains both panel and form will tell the form to show itself. What do you think?

But yeah, wrapping them in a component to restrict dom element scope would still be needed.

Got it. Thanks

mssola · 2017-03-13T16:47:34Z

vendor/assets/javascripts/lifeitup_layout.js

@@ -1,130 +1,130 @@
-// to render the layout correctly in every browser/screen


I'm not sure I understand what changed here 😅

Me neither. Maybe because of the rebase? LOL. 😅

Then, rebase again and checkout -- this file 😉

vitoravelino · 2017-03-13T18:49:45Z

Added some documentation. I would appreciate some guidance since I don't know what you are expecting.

mssola · 2017-03-14T09:19:41Z

@vitoravelino you overdid it! 😄 So, the documentation should be one the same lines as the ruby code: human explanation of what it does, and when there is a parameter explain what is it expecting and all that. You can take the LDAP class as reference 😉 So, avoid all the @ references and such, it should be simpler.

mssola · 2017-03-14T09:21:00Z

Also, for comments I'd only use // instead of /**

vitoravelino · 2017-03-14T10:15:11Z

KKKKK. Will do. 😅

vitoravelino · 2017-03-14T13:07:03Z

@mssola Done. Let me know if it still needs improvement, please.

mssola · 2017-03-14T14:16:25Z

.babelrc

@@ -0,0 +1,3 @@
+{
+  "presets": ["es2015"]
+}


I'm ok with this, but I think that we already provide the babel config through the package.json file no ? Why would we need this ? (asking just in case we can avoid duplication)

I didn't see the package.json config. I prefer having babel config on their own file. This is actually from my work of adding ES2015 support that I forgot to delete it.

I just realized that we might not need both of them since you already set

query: { presets: ['es2015'], },

on the babel-loader config. Should I remove both or just .babelrc?

Maybe just the babelrc file.

mssola · 2017-03-14T14:17:35Z

app/assets/javascripts/base/component.js

@@ -0,0 +1,53 @@
+/* eslint-disable class-methods-use-this */
+
+//


You can remove this line and the one in the end. So, all comments can be like this:

// BaseComponent class to avoid boilerplate code // into component classes and add some convention. // // The subclasses are not obligated to implement all // of these methods but if they need to perform an action // that fits one of the descriptions below, it's highly // recommended to use the proper method.

This same thing applies to the other classes, functions, etc.

mssola · 2017-03-14T14:19:07Z

app/views/devise/registrations/edit.html.slim

-          .form-group
-            .col-md-offset-2.col-md-7
-              = f.submit('Create', class: 'btn btn-primary')
+    .app-token-wrapper


Got it. Thanks

mssola · 2017-03-14T14:19:55Z

vendor/assets/javascripts/lifeitup_layout.js

@@ -1,130 +1,130 @@
-// to render the layout correctly in every browser/screen


Then, rebase again and checkout -- this file 😉

mssola

Just remove the .babelrc file and squash your commits and I'll merge it. Thanks for the patience! 👍

Signed-off-by: Vítor Avelino <[email protected]>

vitoravelino · 2017-03-14T15:46:32Z

No worries, @mssola. 😄

mssola · 2017-03-14T16:08:15Z

Thanks 👏

vitoravelino changed the title ~~ES2015 support~~ [wip] ES2015 support Mar 4, 2017

vitoravelino commented Mar 5, 2017

View reviewed changes

vitoravelino changed the title ~~[wip] ES2015 support~~ ES2015 support Mar 5, 2017

vitoravelino force-pushed the es2015-support branch from 1dbfe64 to 11815ce Compare March 9, 2017 20:25

vitoravelino changed the title ~~ES2015 support~~ users: ES2015 refactoring Mar 9, 2017

vitoravelino force-pushed the es2015-support branch 3 times, most recently from 66a0bbe to 0b93aac Compare March 11, 2017 11:26

mssola suggested changes Mar 13, 2017

View reviewed changes

vitoravelino changed the title ~~users: ES2015 refactoring~~ js: users module refactoring Mar 13, 2017

vitoravelino force-pushed the es2015-support branch 2 times, most recently from c1ec5fa to 72d2839 Compare March 14, 2017 13:05

mssola reviewed Mar 14, 2017

View reviewed changes

vitoravelino force-pushed the es2015-support branch from 72d2839 to 78e8ef6 Compare March 14, 2017 14:26

mssola approved these changes Mar 14, 2017

View reviewed changes

vitoravelino force-pushed the es2015-support branch from 1c9ebc7 to 3b7488a Compare March 14, 2017 15:44

js (users): module refactoring

52bc961

Signed-off-by: Vítor Avelino <[email protected]>

vitoravelino force-pushed the es2015-support branch from 3b7488a to 52bc961 Compare March 14, 2017 15:44

mssola merged commit f524db1 into SUSE:master Mar 14, 2017

vitoravelino mentioned this pull request Mar 15, 2017

Client side validation of Signup form #1251

Closed

vitoravelino deleted the es2015-support branch April 25, 2018 12:01

		@@ -1,130 +1,130 @@
		// to render the layout correctly in every browser/screen

		@@ -0,0 +1,53 @@
		/* eslint-disable class-methods-use-this */

		//

js: users module refactoring #1233

js: users module refactoring #1233

Conversation

vitoravelino commented Mar 4, 2017 • edited Loading

vitoravelino commented Mar 5, 2017

Choose a reason for hiding this comment

vitoravelino commented Mar 5, 2017

mssola commented Mar 6, 2017

vitoravelino commented Mar 6, 2017

mssola commented Mar 9, 2017

vitoravelino commented Mar 10, 2017

vitoravelino commented Mar 10, 2017

vitoravelino commented Mar 11, 2017

mssola left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vitoravelino commented Mar 13, 2017

mssola commented Mar 14, 2017

mssola commented Mar 14, 2017

vitoravelino commented Mar 14, 2017

vitoravelino commented Mar 14, 2017

Choose a reason for hiding this comment

vitoravelino Mar 14, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mssola left a comment

Choose a reason for hiding this comment

vitoravelino commented Mar 14, 2017

mssola commented Mar 14, 2017

vitoravelino commented Mar 4, 2017 •

edited

Loading

mssola left a comment •

edited

Loading

vitoravelino Mar 14, 2017 •

edited

Loading