You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@rave.apache.org by Ankur Goyal <as...@indiana.edu> on 2012/01/11 21:26:03 UTC
Review Request: Addressed Issues related to previous review request:
https://reviews.apache.org/r/3449/
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/3462/
-----------------------------------------------------------
Review request for rave and Marlon Pierce.
Summary
-------
Addressed the following issues:-
-> added test class for UserInfoController
-> added links for home, admin interface and store
-> added Dutch Translations to messages_nl.properties
Yes, I am assuming status to be single/married for now.
Diffs
-----
trunk/rave-components/rave-web/src/main/java/org/apache/rave/portal/web/controller/UserInfoController.java PRE-CREATION
trunk/rave-components/rave-web/src/main/java/org/apache/rave/portal/web/util/ViewNames.java 1230199
trunk/rave-components/rave-web/src/test/java/org/apache/rave/portal/web/controller/UserInfoControllerTest.java PRE-CREATION
trunk/rave-portal-resources/src/main/resources/messages.properties 1230199
trunk/rave-portal-resources/src/main/resources/messages_nl.properties 1230199
trunk/rave-portal-resources/src/main/webapp/WEB-INF/jsp/views/page.jsp 1230199
trunk/rave-portal-resources/src/main/webapp/WEB-INF/jsp/views/profile_body.jsp PRE-CREATION
trunk/rave-portal-resources/src/main/webapp/WEB-INF/jsp/views/profile_footer.jsp PRE-CREATION
trunk/rave-portal-resources/src/main/webapp/WEB-INF/jsp/views/profile_header.jsp PRE-CREATION
trunk/rave-portal-resources/src/main/webapp/WEB-INF/jsp/views/profile_menu.jsp PRE-CREATION
trunk/rave-portal-resources/src/main/webapp/WEB-INF/jsp/views/user_profile.jsp PRE-CREATION
trunk/rave-portal-resources/src/main/webapp/WEB-INF/tiles-defs.xml 1230199
trunk/rave-portal-resources/src/main/webapp/css/default.css 1230199
trunk/rave-portal-resources/src/main/webapp/script/rave_api.js 1230199
trunk/rave-portal-resources/src/main/webapp/script/rave_profile.js PRE-CREATION
Diff: https://reviews.apache.org/r/3462/diff
Testing
-------
Thanks,
Ankur
Re: Review Request: Addressed Issues related to previous review request:
https://reviews.apache.org/r/3449/
Posted by Ankur Goyal <as...@indiana.edu>.
> On 2012-01-11 22:02:56, Marlon Pierce wrote:
> > trunk/rave-portal-resources/src/main/webapp/WEB-INF/jsp/views/page.jsp, line 31
> > <https://reviews.apache.org/r/3462/diff/1/?file=68146#file68146line31>
> >
> > This won't work for new accounts, which don't have display names. Use username instead or else we need to change the new account forms.
I can keep a check on that, like if display name is not provided then use the user name. Similar to what is done in page.jsp header.
> On 2012-01-11 22:02:56, Marlon Pierce wrote:
> > trunk/rave-portal-resources/src/main/webapp/WEB-INF/jsp/views/profile_body.jsp, line 23
> > <https://reviews.apache.org/r/3462/diff/1/?file=68147#file68147line23>
> >
> > What is body content supposed to be?
It can be things like, by default it can show the search button for the user to search friends and if he wants to edit his profile information such as basic information, we can have a link or a button on that tab (menu) which will refresh the body content and make the editor for basic information visible.
But for now I kept it empty as these features are for other tickets.
> On 2012-01-11 22:02:56, Marlon Pierce wrote:
> > trunk/rave-portal-resources/src/main/webapp/WEB-INF/jsp/views/profile_menu.jsp, line 46
> > <https://reviews.apache.org/r/3462/diff/1/?file=68150#file68150line46>
> >
> > Users should be able to update this information.
Shouldn't this be set as a new sub task?
- Ankur
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/3462/#review4318
-----------------------------------------------------------
On 2012-01-11 20:26:03, Ankur Goyal wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/3462/
> -----------------------------------------------------------
>
> (Updated 2012-01-11 20:26:03)
>
>
> Review request for rave and Marlon Pierce.
>
>
> Summary
> -------
>
> Addressed the following issues:-
> -> added test class for UserInfoController
> -> added links for home, admin interface and store
> -> added Dutch Translations to messages_nl.properties
>
> Yes, I am assuming status to be single/married for now.
>
>
> Diffs
> -----
>
> trunk/rave-components/rave-web/src/main/java/org/apache/rave/portal/web/controller/UserInfoController.java PRE-CREATION
> trunk/rave-components/rave-web/src/main/java/org/apache/rave/portal/web/util/ViewNames.java 1230199
> trunk/rave-components/rave-web/src/test/java/org/apache/rave/portal/web/controller/UserInfoControllerTest.java PRE-CREATION
> trunk/rave-portal-resources/src/main/resources/messages.properties 1230199
> trunk/rave-portal-resources/src/main/resources/messages_nl.properties 1230199
> trunk/rave-portal-resources/src/main/webapp/WEB-INF/jsp/views/page.jsp 1230199
> trunk/rave-portal-resources/src/main/webapp/WEB-INF/jsp/views/profile_body.jsp PRE-CREATION
> trunk/rave-portal-resources/src/main/webapp/WEB-INF/jsp/views/profile_footer.jsp PRE-CREATION
> trunk/rave-portal-resources/src/main/webapp/WEB-INF/jsp/views/profile_header.jsp PRE-CREATION
> trunk/rave-portal-resources/src/main/webapp/WEB-INF/jsp/views/profile_menu.jsp PRE-CREATION
> trunk/rave-portal-resources/src/main/webapp/WEB-INF/jsp/views/user_profile.jsp PRE-CREATION
> trunk/rave-portal-resources/src/main/webapp/WEB-INF/tiles-defs.xml 1230199
> trunk/rave-portal-resources/src/main/webapp/css/default.css 1230199
> trunk/rave-portal-resources/src/main/webapp/script/rave_api.js 1230199
> trunk/rave-portal-resources/src/main/webapp/script/rave_profile.js PRE-CREATION
>
> Diff: https://reviews.apache.org/r/3462/diff
>
>
> Testing
> -------
>
>
> Thanks,
>
> Ankur
>
>
Re: Review Request: Addressed Issues related to previous review request:
https://reviews.apache.org/r/3449/
Posted by Marlon Pierce <mp...@cs.indiana.edu>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/3462/#review4318
-----------------------------------------------------------
trunk/rave-portal-resources/src/main/webapp/WEB-INF/jsp/views/page.jsp
<https://reviews.apache.org/r/3462/#comment9724>
This won't work for new accounts, which don't have display names. Use username instead or else we need to change the new account forms.
trunk/rave-portal-resources/src/main/webapp/WEB-INF/jsp/views/profile_body.jsp
<https://reviews.apache.org/r/3462/#comment9720>
What is body content supposed to be?
trunk/rave-portal-resources/src/main/webapp/WEB-INF/jsp/views/profile_header.jsp
<https://reviews.apache.org/r/3462/#comment9725>
Note the widget store and admin interface header.jsp files should be modified to also have a link to the profile. Or better, all of these header pages should be consolidated.
trunk/rave-portal-resources/src/main/webapp/WEB-INF/jsp/views/profile_header.jsp
<https://reviews.apache.org/r/3462/#comment9718>
New accounts don't have display names. Probably this should be added to the New Account form, so not a problem with the current patch.
trunk/rave-portal-resources/src/main/webapp/WEB-INF/jsp/views/profile_menu.jsp
<https://reviews.apache.org/r/3462/#comment9719>
I'd like to see something nicer looking, like the Google+ profile layout: "about", "posts", "friends", etc. We only have about information now, but I suggest laying out the full profile. We can disable parts for now and then fill in later.
trunk/rave-portal-resources/src/main/webapp/WEB-INF/jsp/views/profile_menu.jsp
<https://reviews.apache.org/r/3462/#comment9721>
Users should be able to update this information.
trunk/rave-portal-resources/src/main/webapp/WEB-INF/jsp/views/user_profile.jsp
<https://reviews.apache.org/r/3462/#comment9726>
(Again) I think you want to go ahead and make a full profile layout (Facebook or G+ style) and use placeholders for features that aren't available yet. It would look nicer.
trunk/rave-portal-resources/src/main/webapp/script/rave_profile.js
<https://reviews.apache.org/r/3462/#comment9722>
Need to add jasmine tests.
trunk/rave-portal-resources/src/main/webapp/script/rave_profile.js
<https://reviews.apache.org/r/3462/#comment9723>
Shouldn't use hard coded background color.
- Marlon
On 2012-01-11 20:26:03, Ankur Goyal wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/3462/
> -----------------------------------------------------------
>
> (Updated 2012-01-11 20:26:03)
>
>
> Review request for rave and Marlon Pierce.
>
>
> Summary
> -------
>
> Addressed the following issues:-
> -> added test class for UserInfoController
> -> added links for home, admin interface and store
> -> added Dutch Translations to messages_nl.properties
>
> Yes, I am assuming status to be single/married for now.
>
>
> Diffs
> -----
>
> trunk/rave-components/rave-web/src/main/java/org/apache/rave/portal/web/controller/UserInfoController.java PRE-CREATION
> trunk/rave-components/rave-web/src/main/java/org/apache/rave/portal/web/util/ViewNames.java 1230199
> trunk/rave-components/rave-web/src/test/java/org/apache/rave/portal/web/controller/UserInfoControllerTest.java PRE-CREATION
> trunk/rave-portal-resources/src/main/resources/messages.properties 1230199
> trunk/rave-portal-resources/src/main/resources/messages_nl.properties 1230199
> trunk/rave-portal-resources/src/main/webapp/WEB-INF/jsp/views/page.jsp 1230199
> trunk/rave-portal-resources/src/main/webapp/WEB-INF/jsp/views/profile_body.jsp PRE-CREATION
> trunk/rave-portal-resources/src/main/webapp/WEB-INF/jsp/views/profile_footer.jsp PRE-CREATION
> trunk/rave-portal-resources/src/main/webapp/WEB-INF/jsp/views/profile_header.jsp PRE-CREATION
> trunk/rave-portal-resources/src/main/webapp/WEB-INF/jsp/views/profile_menu.jsp PRE-CREATION
> trunk/rave-portal-resources/src/main/webapp/WEB-INF/jsp/views/user_profile.jsp PRE-CREATION
> trunk/rave-portal-resources/src/main/webapp/WEB-INF/tiles-defs.xml 1230199
> trunk/rave-portal-resources/src/main/webapp/css/default.css 1230199
> trunk/rave-portal-resources/src/main/webapp/script/rave_api.js 1230199
> trunk/rave-portal-resources/src/main/webapp/script/rave_profile.js PRE-CREATION
>
> Diff: https://reviews.apache.org/r/3462/diff
>
>
> Testing
> -------
>
>
> Thanks,
>
> Ankur
>
>
Re: Review Request: Addressed Issues related to previous review request:
https://reviews.apache.org/r/3449/
Posted by Jesse Ciancetta <jc...@mitre.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/3462/#review4359
-----------------------------------------------------------
Oops -- I didn't realize that this review request was a follow up to a previously posted review request -- I just added comments on the previous review request here:
https://reviews.apache.org/r/3449/
although I think those comments still apply.
You should be able to respond to feedback and post subsequent patches without needing to create an entirely new review -- let us know if you have trouble finding the right places to do that and we can help out.
- Jesse
On 2012-01-11 20:26:03, Ankur Goyal wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/3462/
> -----------------------------------------------------------
>
> (Updated 2012-01-11 20:26:03)
>
>
> Review request for rave and Marlon Pierce.
>
>
> Summary
> -------
>
> Addressed the following issues:-
> -> added test class for UserInfoController
> -> added links for home, admin interface and store
> -> added Dutch Translations to messages_nl.properties
>
> Yes, I am assuming status to be single/married for now.
>
>
> Diffs
> -----
>
> trunk/rave-components/rave-web/src/main/java/org/apache/rave/portal/web/controller/UserInfoController.java PRE-CREATION
> trunk/rave-components/rave-web/src/main/java/org/apache/rave/portal/web/util/ViewNames.java 1230199
> trunk/rave-components/rave-web/src/test/java/org/apache/rave/portal/web/controller/UserInfoControllerTest.java PRE-CREATION
> trunk/rave-portal-resources/src/main/resources/messages.properties 1230199
> trunk/rave-portal-resources/src/main/resources/messages_nl.properties 1230199
> trunk/rave-portal-resources/src/main/webapp/WEB-INF/jsp/views/page.jsp 1230199
> trunk/rave-portal-resources/src/main/webapp/WEB-INF/jsp/views/profile_body.jsp PRE-CREATION
> trunk/rave-portal-resources/src/main/webapp/WEB-INF/jsp/views/profile_footer.jsp PRE-CREATION
> trunk/rave-portal-resources/src/main/webapp/WEB-INF/jsp/views/profile_header.jsp PRE-CREATION
> trunk/rave-portal-resources/src/main/webapp/WEB-INF/jsp/views/profile_menu.jsp PRE-CREATION
> trunk/rave-portal-resources/src/main/webapp/WEB-INF/jsp/views/user_profile.jsp PRE-CREATION
> trunk/rave-portal-resources/src/main/webapp/WEB-INF/tiles-defs.xml 1230199
> trunk/rave-portal-resources/src/main/webapp/css/default.css 1230199
> trunk/rave-portal-resources/src/main/webapp/script/rave_api.js 1230199
> trunk/rave-portal-resources/src/main/webapp/script/rave_profile.js PRE-CREATION
>
> Diff: https://reviews.apache.org/r/3462/diff
>
>
> Testing
> -------
>
>
> Thanks,
>
> Ankur
>
>