Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Kitchensink backbone #466

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
4 participants
Contributor

joshuawilson commented Mar 6, 2013

I created a Backbone quickstart based on the kitchensink HTML5 quickstart.

It is ready to go. If I missed anything please let me know and I will fix as soon as possible.

thanks,
Joshua

Contributor

pmuir commented Mar 7, 2013

@VineetReynolds could you do the tech review on this quickstart when you have time?

Member

VineetReynolds commented Mar 8, 2013

@pmuir Yes, definitely.

Member

VineetReynolds commented Mar 15, 2013

Hey all, this is just to let you know I've started reviewing this.

@VineetReynolds VineetReynolds commented on the diff Mar 15, 2013

kitchensink-backbone/src/main/webapp/index.html
+ <!-- Set viewport scaling and zoom features -->
+ <meta name="viewport" content="width=device-width, initial-scale=1">
+
+ <script type="text/javascript" src="js/libs/modernizr-2.6.2.min.js"></script>
+
+ <script type="text/javascript">
+ Modernizr.load([{
+ //Load CDN hosted jQuery or fall back to local file.
+ /*--------Note-----------
+ This approach should only be used if you trust the source of where the resource (JavaScript) is located.
+ In this case, use of the jquery CDN is much faster if your user's browser already has this file cached.
+ If they don't have it cached, try to load it from the CDN as this will be faster if the user is
+ located closer to a CDN than you currently hosted server.
+ */
+ load: [
+ "http://code.jquery.com/jquery-1.9.1.js",
@VineetReynolds

VineetReynolds Mar 15, 2013

Member

jQuery 1.9.1 is "incompatible" with jQuery Mobile 1.2.0. Loading jQM 1.2.0 errors out with jQ 1.9.1. I'm basing this off a very simple test of resizing the browser window to suit the mobile viewport size (a refresh may be required). The app fails to display the jQM styles on my Android as well, for a realistic test. As a tip, you could use the HTML5-Mobile kitchen sink app to establish expected behavior, like so: http://imgur.com/26MXqAd

For reference: the supported jQuery versions (> 1.6, <= 1.8.2) for jQM 1.2.0 is documented at http://jquerymobile.com/blog/2012/10/02/announcing-jquery-mobile-1-2-0-final/ and for jQM 1.3.0 at http://jquerymobile.com/blog/2013/02/20/jquery-mobile-1-3-0-released/

If we do bump up the version of jQ+jQM, then, in my opinion, we should do this for all kitchensink apps.

Note - The mobile styles are applied when I downgrade jQuery to 1.8.2, or upgrade jQM to 1.3.0. So it's just a matter of deciding a downgrade or an upgrade and running/writing additional visual consistency tests.

@joshuawilson

joshuawilson Mar 16, 2013

Contributor

Good catch, thanks Vineet. I added the jQuery Mobile 1.3.0. It is my opinion that we should be on the latest version of libraries. At some point I figure I can update the HTML/mobile project too.

Pete if you want these downgraded for some reason, I can do that too.

@VineetReynolds VineetReynolds and 1 other commented on an outdated diff Mar 16, 2013

kitchensink-backbone/src/main/webapp/index.html
+ <div class="col">
+ <div class="head">Name</div>
+ </div>
+ <div class="col">
+ <div class="head">Email</div>
+ </div>
+ <div class="col">
+ <div class="head">Phone #</div>
+ </div>
+ <div class="col">
+ <div class="head">REST URL</div>
+ </div>
+</div>
+ </script>
+
+ <script type="text/template" id="member-Body-tmpl">
@VineetReynolds

VineetReynolds Mar 16, 2013

Member

This Underscore template works correctly in the desktop view, but not in the mobile view. You may want to add in evaluations involving Modernizr to add the headers divs, like in this template: https://github.com/aerogear/as-quickstarts/blob/master/kitchensink-html5-mobile/src/main/webapp/index.html#L351

You may also need to combine this template with the "member-Header-Row-tmpl" template above, but that might require reconsidering the roles of the MemberView and the AppView.

@VineetReynolds

VineetReynolds Mar 16, 2013

Member

Oops, I meant to provide this link to https://github.com/jboss-jdf/jboss-as-quickstart/blob/master/kitchensink-html5-mobile/src/main/webapp/index.html#L351 instead of the one in the Aerogear git repo, in the previous comment.

@joshuawilson

joshuawilson Mar 16, 2013

Contributor

I ended up changing so much of the template to get it to display without refreshing in the desktop browser view that I forgot the mobile view. Thanks for pointing this out. It should be fixed now.

@VineetReynolds VineetReynolds commented on the diff Mar 16, 2013

kitchensink-backbone/src/main/webapp/js/app.js
+ *
+ * @Author: Joshua Wilson
+ */
+
+/* Builds the updated table for the member list */
+// Load the application once the DOM is ready, using `jQuery.ready`:
+$(function() {
+ /*
+ * Models are the heart of any JavaScript application, containing the
+ * interactive data as well as a large part of the logic surrounding it:
+ * conversions, validations, computed properties, and access control.
+ * You extend Backbone.Model with your domain-specific methods, and Model
+ * provides a basic set of functionality for managing changes.
+ */
+ // Our basic **Member** model
+ window.Member = Backbone.Model.extend({
@VineetReynolds

VineetReynolds Mar 16, 2013

Member

I'm undecided on whether it is a good thing to do bind the Backbone models, collections and views to the window objects, in a kitchensink app. This is not a bad thing considering that the app has just one model, one collection and a couple of views. This is also the case in the Todos app.

But then, it would be better to create a namespace for the app, containing the models, collections and views as properties, if we were to use best practices here. Ref: http://ricostacruz.com/backbone-patterns/#namespace_convention

@joshuawilson

joshuawilson Mar 16, 2013

Contributor

I will have to look more into this one. Thanks for bringing it up.

@VineetReynolds VineetReynolds and 1 other commented on an outdated diff Mar 16, 2013

kitchensink-backbone/pom.xml
+ <license>
+ <name>Apache License, Version 2.0</name>
+ <distribution>repo</distribution>
+ <url>http://www.apache.org/licenses/LICENSE-2.0.html</url>
+ </license>
+ </licenses>
+
+ <dependencyManagement>
+ <dependencies>
+
+ <!-- JBoss distributes a complete set of Java EE 6 APIs including
+ a Bill of Materials (BOM). A BOM specifies the versions of a "stack" (or
+ a collection) of artifacts. We use this here so that we always get the correct
+ versions of artifacts. Here we use the jboss-javaee-web-6.0 stack (you can
+ read this as the JBoss stack of the Java EE Web Profile 6 APIs) -->
+ <dependency>
@VineetReynolds

VineetReynolds Mar 16, 2013

Member

This managed dependency is likely to be the incorrect one to be used. I could not get the Arquillian tests to run, since the version of the adapter is not provided via this dependency. The kitchensink-angularjs quickstart uses a different set of managed dependencies. Using those makes the problem go away, since they provide the Arquillian JARs with versions. Using that managed dependency also ensures that the non-existent 7.3.Final Arquillian managed AS7 adapter is not used.

Ref: https://github.com/jboss-jdf/jboss-as-quickstart/blob/master/kitchensink-angularjs/pom.xml#L76 for the AngularJS kitchensink quickstart.

@joshuawilson

joshuawilson Mar 16, 2013

Contributor

This entire project was taken as a copy from the HTML/mobile Aerogear one. So I just used the it's pom.xml. That said, I see your point and update the pom to match what Pete did in the AngularJS project. Thanks for pointing it out.

@VineetReynolds VineetReynolds and 1 other commented on an outdated diff Mar 16, 2013

kitchensink-backbone/pom.xml
+ <version>7.1.2-SNAPSHOT</version>
+ <packaging>war</packaging>
+
+ <name>JBoss AS Quickstarts: Backbone</name>
+ <description>A Backbone.js and Java EE 6 HTML5 mobile web application for use with JBoss.</description>
+
+ <properties>
+ <!-- Explicitly declaring the source encoding eliminates the following message: -->
+ <!-- [WARNING] Using platform encoding (UTF-8 actually) to copy filtered resources, i.e. build is platform dependent! -->
+ <project.build.sourceEncoding>UTF-8</project.build.sourceEncoding>
+
+ <!-- You can reference property in pom.xml or filtered resources (must enable third-party plugin if using Maven < 2.1) -->
+
+ <!-- JBoss dependency versions -->
+
+ <version.org.jboss.as>7.3.Final</version.org.jboss.as>
@VineetReynolds

VineetReynolds Mar 16, 2013

Member

I'm not entirely sure if this is correct. Maybe it should be 7.1.3.Final ?

@VineetReynolds

VineetReynolds Mar 16, 2013

Member

My bad. 7.3.Final makes sense for the Maven plugin. But not certainly for the Arquillian AS7 adapter version.

@joshuawilson

joshuawilson Mar 16, 2013

Contributor

With the clean up of the pom.xml I think this is OBE.

@VineetReynolds VineetReynolds commented on the diff Mar 16, 2013

kitchensink-backbone/src/test/qunit/test/test.js
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+/*
+Unit tests that cover basic functionality of app.js.
+ */
+
+module('Member Row Construction');
+
+test('Build 2 Member Rows', function() {
+ expect(1);
+
+ var members = [{"email": "jane.doe@company.com", "id": 1, "name": "Jane Doe", "phoneNumber": "12312312311"},{"email": "john.doe@company.com", "id": 0, "name": "John Doe", "phoneNumber": "2125551212"}];
+
+ var html = buildMemberRows(members);
@VineetReynolds

VineetReynolds Mar 16, 2013

Member

This QUnit test fails at this point, since the buildMemberRows() function is not present in the app.js file. Perhaps the App.addOneMember() or App.addAllMembers() function should be invoked instead.

@joshuawilson

joshuawilson Mar 16, 2013

Contributor

Yeah, that test won't work. I didn't even both changing it. Adding in all the test cases for a backbone app is time consuming. It is on my list of things to do. You can't just reference a function like normal JavaScript. You need to Mock and Stub it with something like SinonJS. I'll post when I get these written.

@VineetReynolds

VineetReynolds Mar 16, 2013

Member

You're right about considering whether to mock out the REST services and test the models and collections in isolation. I didn't want to push for SinonJS since I've merely read about it's use for testing Backbone.js apps.

But then, I've looked at the other quickstarts on how they incorporate JavaScript tests. I've filed JDF-238 for the kitchensink-html5-mobile app. I believe that is relevant here, because both of the quickstarts should employ more or less the same approach for QUnit tests. Also to be noted is that the kitchensink-angularjs has no QUnit tests (or for that matter Jasmine or Angular Scenario based tests).

Therefore, we should set out the scope of what QUnit tests should do - unit, integration or end-to-end tests? And then look at implementing tests as desired.

@VineetReynolds VineetReynolds commented on an outdated diff Mar 16, 2013

kitchensink-backbone/src/test/qunit/index.html
+
+-->
+<!DOCTYPE html>
+
+<!--
+Base test file for running quit tests. Load this into any browser to execute
+the tests for the HTML5/Mobile application.
+-->
+
+<html>
+<head>
+ <meta charset="UTF-8" />
+ <title>HTML5 Test Suite</title>
+
+ <link rel="stylesheet" href="qunit/qunit.css" type="text/css" media="screen">
+ <script type="text/javascript" src="../../main/webapp/js/libs/jquery-1.8.2.min.js"></script>
@VineetReynolds

VineetReynolds Mar 16, 2013

Member

Some of these paths are incorrect - the jQuery version issue is known (see the other comment on 1.8 vs 1.9 usage). Lodash path needs correction.

Backbone.js and jQuery Mobile also need to be added.

@sgilda sgilda commented on an outdated diff Mar 28, 2013

kitchensink-backbone/README.md
@@ -0,0 +1,95 @@
+kitchensink-backbone: Shows how to use Backbone.js with JAX-RS and Java EE on JBoss
+========================
+Author: Joshua Wilson
+Level: Intermediate
+Technologies: Backbone, CDI, JPA, EJB, JPA, JAX-RS, BV
+Summary: An example that incorporates multiple technologies
+Target Product: EAP
+

@sgilda sgilda commented on an outdated diff Mar 28, 2013

kitchensink-backbone/README.md
@@ -0,0 +1,95 @@
+kitchensink-backbone: Shows how to use Backbone.js with JAX-RS and Java EE on JBoss
+========================
+Author: Joshua Wilson
+Level: Intermediate
+Technologies: Backbone, CDI, JPA, EJB, JPA, JAX-RS, BV
+Summary: An example that incorporates multiple technologies
+Target Product: EAP
+
+What is it?
+-----------
+
+This is your project! It is a sample, deployable Maven 3 project to help you get your foot in the door developing with Backbone.js on Java EE 6 on JBoss Enterprise Application Platform 6 or JBoss AS 7.
+
+This project is setup to allow you to create a compliant Java EE 6 application using CDI 1.0, EJB 3.1, JPA 2.0 and Bean Validation 1.0. It includes a persistence unit and some sample persistence and transaction code to introduce you to database access in enterprise Java.
+
@sgilda

sgilda Mar 28, 2013

Contributor

This is basically the same as the description in the kitchensink quickstart. It might be nice to say this example uses the kitchensink as its starting point but add about what is Backbone.js, how this quicksart uses it, what are the benefits, maybe point out changes that were made to use Backbone that make it differ from kitchensink.

Contributor

sgilda commented Mar 28, 2013

This works great! I have a few general comments:

Error handling is confusing.

  • When I type in an email address with an invalid format, it just displays "Please enter an email address"
  • When I type in an invalid phone number (too short or non-numeric) it displays: "Please match the requested format.", but you have no idea what the requested format is.

I ran the qstool checker and it turned up a quite a few violations.

  • None of the Java files have the correct indentation or improper formatting of curly braces, some files are missing licenses, and other contain tab characters. Could you run them through the formatter? I'm not sure the formatter replaces tab characters though, so you'll need to verify.
  • The following 5 properties defined in the pom.xml file are not used and should be removed:
    version.org.hibernate-jpamodelgen
    version.org.jboss.spec.jboss.javaee.6.0
    version.org.hibernate.validator
    version.junit
    version.org.jboss.arquillian
Contributor

sgilda commented Apr 3, 2013

@joshuawilson : Also, when the changes are completed, would you mind squashing it down to 1 commit? ;-)

Contributor

joshuawilson commented Apr 3, 2013

Sure thing. I saw your feedback, thanks. I haven't been able to get to it yet due to my current work load. I'll try to do some tonight.

Contributor

sgilda commented Apr 3, 2013

No problem at all. Thanks!

Contributor

pmuir commented Apr 26, 2013

@joshuawilson @sgilda is this ready to merge?

Contributor

sgilda commented Apr 26, 2013

@pmuir I think we are waiting for @joshuawilson to make the formatting changes and POM fixes mentioned in my comment above .

Contributor

joshuawilson commented Apr 26, 2013

I have about half the changes made but I have not committed them. I will set aside some time today to finish this.

Contributor

sgilda commented May 8, 2013

@joshuawilson : Just checking to see where this stands with the formatting and POM fixes. :-)

Contributor

joshuawilson commented May 9, 2013

@sgilda: I made the changes to the POM. Updated the README. Ran the qtools. And I changed the error messages. Then I squashed it.

@sgilda sgilda and 1 other commented on an outdated diff May 10, 2013

kitchensink-backbone/src/main/webapp/index.html
+ this.setCustomValidity('');
+ if (!this.validity.valid) {
+ this.setCustomValidity("Dude '" + this.value + "' is not a valid email. Try something like "+
+ "jim@jboss.org. And no we are not checking if it actually works, we are just looking "+
+ "for the @ sign. ");
+ }
+ });
+
+ //Listen for the 'input' event on the phone number field. If HTML5 validation throws a typeMismatch, then
+ // override the default message with something more useful and/or sarcastic.
+ $('#phoneNumber').bind('input', function () {
+ //We need to reset it to blank or it will throw an invalid message.
+ this.setCustomValidity('');
+ if (!this.validity.valid) {
+ this.setCustomValidity("Did you really think that '" + this.value + "' is a phone number. "+
+ "No one uses a dash when they dial and neither do we. Try something like 5551234567. ");
@sgilda

sgilda May 10, 2013

Contributor

Love the messages, but this needs very minor edits (mainly because when I type 'x's in the input, the message doesn't sound right):

                  this.setCustomValidity("Did you really think that '" + this.value + "' is a phone number? "+
                             "No one uses characters or dashes when they dial and neither do we. Try something like 5551234567. ");
@joshuawilson

joshuawilson May 10, 2013

Contributor

Thanks for the review and I agree with the changes and made them.

Contributor

sgilda commented May 10, 2013

Joshua,
Looks good! Got a good laugh out of the validation messages! I made a minor comment on one to make it a little clearer and change the period to a question mark. Other than that, I think this is good to go!

@pmuir or @VineetReynolds : Did you need to do a last review?

Contributor

sgilda commented May 10, 2013

Thanks! Waiting to see if @pmuir or @VineetReynolds need to do a final review.

Member

VineetReynolds commented May 28, 2013

Merged.

I made some corrections on top of Joshua's work from last week and pushed them upstream: cd0a9fd

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment