Skip to content
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

Grails Kitchensink App added #560

Closed
wants to merge 1 commit into from

Conversation

TejasM
Copy link
Contributor

@TejasM TejasM commented Jul 9, 2013

No description provided.

@sgilda
Copy link
Contributor

sgilda commented Jul 10, 2013

@TejasM : Is there a reason class files are checked into Git on this commit?

@sgilda
Copy link
Contributor

sgilda commented Jul 10, 2013

This builds and runs great! However, the form has 3 input fields without labels, so it's not clear what you need to enter in the fields. I had to guess by clicking the button and getting errors. :-)

@sgilda
Copy link
Contributor

sgilda commented Jul 10, 2013

Apparently the classes are not generated and are required. Just curious why they aren't in a JAR?

Also, just curious about the empty directories, for example: lib/, src/groovy/, src/java/

@TejasM
Copy link
Contributor Author

TejasM commented Jul 10, 2013

@sgilda: Thx for the starting the review! I made the mistake of relying on the master gitignore that is why binary files got added. I think I have fixed that up, but will check to make sure. I will go through the other mistakes and fix those soon as well.

@sgilda
Copy link
Contributor

sgilda commented Jul 10, 2013

I think you may have lost the README changes with the commit. :-)

@TejasM
Copy link
Contributor Author

TejasM commented Jul 10, 2013

Lucky I had them in my email, so I think I got most of the fixes in at this point. And for the empty src/java and src/groovy, I think they are add by the grails plugin for files that might not be part of the grails-app itself.

@sgilda
Copy link
Contributor

sgilda commented Jul 10, 2013

OK. Sounds good. :-)

@sgilda
Copy link
Contributor

sgilda commented Jul 10, 2013

@TejasM
When you run the application, it displays "You have successfully deployed a basic Spring MVC web application".
Should it say "You have successfully deployed a Grails web application"?

@TejasM
Copy link
Contributor Author

TejasM commented Jul 10, 2013

@sgilda: Yep that would indeed make sense :)

@sgilda
Copy link
Contributor

sgilda commented Jul 16, 2013

Looks good!

@pmuir : Do you need to do a code review?

@pmuir
Copy link
Contributor

pmuir commented Jul 29, 2013

@sgilda, this is one for @joshuaw to review.

@joshuawilson
Copy link

@sgilda @pmuir Sure I can review it.

@joshuawilson
Copy link

@TejasM The errors on the UI do not display cleanly.

If you leave everything blank and try to Register, the errors for Name and Email list the class.

@joshuawilson
Copy link

@TejasM The Phone Number field does not have validation on it when left blank.

The validation on what is typed works.

@joshuawilson
Copy link

@TejasM

I found the following errors in the pom.xml in Eclipse with both JBDS and the Grails plugin installed.
It is worth noting that it works from the command line.

- Plugin execution not covered by lifecycle configuration: org.grails:grails-maven-plugin:2.2.3:config-directories (execution: default-config-directories, phase: generate-
 sources)
- Plugin execution not covered by lifecycle configuration: org.grails:grails-maven-plugin:2.2.3:maven-compile (execution: default-maven-compile, phase: compile)
- Plugin execution not covered by lifecycle configuration: org.grails:grails-maven-plugin:2.2.3:validate (execution: default-validate, phase: validate)
- Plugin execution not covered by lifecycle configuration: org.grails:grails-maven-plugin:2.2.3:init (execution: default-init, phase: initialize)

@joshuawilson
Copy link

@TejasM Most of the .groovy code is missing comments and licenses.

@sgilda
Copy link
Contributor

sgilda commented Aug 21, 2013

Comment from @joshuawilson on IRC last evening:

    I just talked with Tejas and he will not be able to get Booking or Grails fixed before the re-org

@sgilda
Copy link
Contributor

sgilda commented Aug 21, 2013

Closing for now. Will reopen after reorg.

@sgilda sgilda closed this Aug 21, 2013
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.

4 participants