6:20 a.m.
Hey guys,
Viliam asked me to forward his review notes/remarks here:
Nice job, Viliam, I will incorporate your suggestions.
Btw the overall state of UX improvements is tracked here:
https://issues.jboss.org/browse/AGPUSH-671
Cheers,
~ Lukas
-------- Original Message --------Subject:AG Console review Date:Wed, 28
May 2014 15:01:19 +0200From:Viliam Rockai <vrockai(a)redhat.com> To:Matthias
Wessendorf <mwessend(a)redhat.com>, Lukas Fryc <lfryc(a)redhat.com> CC:Alexandre
Mendonca <amendonc(a)redhat.com>
Hi,
I went through the console code. Overall, it looks just fine. Previously
I thought I'll just put comments into the code and make a commit. Since
I found only a few issues, it's not worth it. So here is my list:
* The name of your angularjs module is "newadminApp". I think it should
be something like "agconsole".
* You're missing feedback for user on many places. If the error says
'Something went wrong...', it doesn't say much to a user. Even changing
it to denote the current operation would be better, something like
'Unable to create new application'. On most of the $resource.* methods
you're missing the feedback, too. For most of them the information about
success would be annoying, but maybe it could be interesting for user to
see errors like 'Unable to get the application list, check your
connection to the server...'. When my session to the console timed-out,
the operation I was trying to do (renaming of app) didn't do anything,
but I had no clue why.
* AFAIK it's considered a bad practice to use jQuery inside controllers.
You use only two $ functions: $.extend and $.ajax, which can be replaced
with angular.extend/angular.copy and the $resource service. If you can't
find a way how to do something without jQuery inside controller, maybe
creating of custom directive is the right solution, but this doesn't
seem to be the case.
* It seems that you're using several switch statements just to assign a
value based on input. This seems to be a good candidate for using a map.
* Notifications - they are inside ng-view pages. It looks fine and works
for your current purposes. But if you make some new page in the future,
which redirects you (i.e. creating a new entity, after submit, user is
redirected to entity list with newly created stuff), you may loose the
message. Another disadvantage of this is non-consistent L&F for feedback
messages. The advantage is, that you're not covering any content with
the message as LO does.
* In installation.html I see stuff like <col width="45%">. I think this
should be rewritten to use the bootstrap grid layout (classes like
col-md-12...).
* We use our custom directive lo-autofocus for the 1st input in modals.
Right after the modal pops-up, user has the focus on the first input and
pressing enter submits the modal.
* The last one is not a problem at all, I just don't feel right about
it :) You use $window.location.href instead $location.absUrl(). They
seem to do the same, but since you don't use any other $window stuff,
using $location makes more sense to me.
Btw, we were thinking about creating a dynamic creation of breadcrumbs,
too. But since we didn't want to have them fully dependent on the URL
(URLs could change for us in the future), we didn't do it. Are you sure
you want to make url and breadcrumbs to be linked this way?
Anyway, congrats, you did good job with the console and this review
helped me to get aware of some problems the LO console may have, too.
Viliam
11:36 a.m.
This is great feedback - thank you Viliam
On May 29, 2014, at 7:20 AM, Lukáš Fryč <lukas.fryc(a)gmail.com> wrote:
Hey guys,
Viliam asked me to forward his review notes/remarks here:
Nice job, Viliam, I will incorporate your suggestions.
Btw the overall state of UX improvements is tracked here:
https://issues.jboss.org/browse/AGPUSH-671
Cheers,
~ Lukas
-------- Original Message --------
Subject: AG Console review
Date: Wed, 28 May 2014 15:01:19 +0200
From: Viliam Rockai <vrockai(a)redhat.com>
To: Matthias Wessendorf <mwessend(a)redhat.com>, Lukas Fryc <lfryc(a)redhat.com>
CC: Alexandre Mendonca <amendonc(a)redhat.com>
Hi,
I went through the console code. Overall, it looks just fine. Previously
I thought I'll just put comments into the code and make a commit. Since
I found only a few issues, it's not worth it. So here is my list:
* The name of your angularjs module is "newadminApp". I think it should
be something like "agconsole".
* You're missing feedback for user on many places. If the error says
'Something went wrong...', it doesn't say much to a user. Even changing
it to denote the current operation would be better, something like
'Unable to create new application'. On most of the $resource.* methods
you're missing the feedback, too. For most of them the information about
success would be annoying, but maybe it could be interesting for user to
see errors like 'Unable to get the application list, check your
connection to the server...'. When my session to the console timed-out,
the operation I was trying to do (renaming of app) didn't do anything,
but I had no clue why.
* AFAIK it's considered a bad practice to use jQuery inside controllers.
You use only two $ functions: $.extend and $.ajax, which can be replaced
with angular.extend/angular.copy and the $resource service. If you can't
find a way how to do something without jQuery inside controller, maybe
creating of custom directive is the right solution, but this doesn't
seem to be the case.
* It seems that you're using several switch statements just to assign a
value based on input. This seems to be a good candidate for using a map.
* Notifications - they are inside ng-view pages. It looks fine and works
for your current purposes. But if you make some new page in the future,
which redirects you (i.e. creating a new entity, after submit, user is
redirected to entity list with newly created stuff), you may loose the
message. Another disadvantage of this is non-consistent L&F for feedback
messages. The advantage is, that you're not covering any content with
the message as LO does.
* In installation.html I see stuff like <col width="45%">. I think this
should be rewritten to use the bootstrap grid layout (classes like
col-md-12...).
* We use our custom directive lo-autofocus for the 1st input in modals.
Right after the modal pops-up, user has the focus on the first input and
pressing enter submits the modal.
* The last one is not a problem at all, I just don't feel right about
it :) You use $window.location.href instead $location.absUrl(). They
seem to do the same, but since you don't use any other $window stuff,
using $location makes more sense to me.
Btw, we were thinking about creating a dynamic creation of breadcrumbs,
too. But since we didn't want to have them fully dependent on the URL
(URLs could change for us in the future), we didn't do it. Are you sure
you want to make url and breadcrumbs to be linked this way?
Anyway, congrats, you did good job with the console and this review
helped me to get aware of some problems the LO console may have, too.
Viliam
_______________________________________________
aerogear-dev mailing list
aerogear-dev(a)lists.jboss.org
https://lists.jboss.org/mailman/listinfo/aerogear-dev
2:09 p.m.
On May 29, 2014, at 12:36 PM, Burr Sutter <bsutter(a)redhat.com> wrote:
This is great feedback - thank you Viliam
+1 thanks a lot Villiam!
On May 29, 2014, at 7:20 AM, Lukáš Fryč <lukas.fryc(a)gmail.com> wrote:
> Hey guys,
>
> Viliam asked me to forward his review notes/remarks here:
>
> Nice job, Viliam, I will incorporate your suggestions.
>
> Btw the overall state of UX improvements is tracked here:
>
> https://issues.jboss.org/browse/AGPUSH-671
>
>
> Cheers,
>
> ~ Lukas
>
>
>
> -------- Original Message --------
> Subject: AG Console review
> Date: Wed, 28 May 2014 15:01:19 +0200
> From: Viliam Rockai <vrockai(a)redhat.com>
> To: Matthias Wessendorf <mwessend(a)redhat.com>, Lukas Fryc
<lfryc(a)redhat.com>
> CC: Alexandre Mendonca <amendonc(a)redhat.com>
>
>
> Hi,
>
> I went through the console code. Overall, it looks just fine. Previously
> I thought I'll just put comments into the code and make a commit. Since
> I found only a few issues, it's not worth it. So here is my list:
>
> * The name of your angularjs module is "newadminApp". I think it should
> be something like "agconsole".
>
> * You're missing feedback for user on many places. If the error says
> 'Something went wrong...', it doesn't say much to a user. Even changing
> it to denote the current operation would be better, something like
> 'Unable to create new application'. On most of the $resource.* methods
> you're missing the feedback, too. For most of them the information about
> success would be annoying, but maybe it could be interesting for user to
> see errors like 'Unable to get the application list, check your
> connection to the server...'. When my session to the console timed-out,
> the operation I was trying to do (renaming of app) didn't do anything,
> but I had no clue why.
>
> * AFAIK it's considered a bad practice to use jQuery inside controllers.
> You use only two $ functions: $.extend and $.ajax, which can be replaced
> with angular.extend/angular.copy and the $resource service. If you can't
> find a way how to do something without jQuery inside controller, maybe
> creating of custom directive is the right solution, but this doesn't
> seem to be the case.
>
> * It seems that you're using several switch statements just to assign a
> value based on input. This seems to be a good candidate for using a map.
>
> * Notifications - they are inside ng-view pages. It looks fine and works
> for your current purposes. But if you make some new page in the future,
> which redirects you (i.e. creating a new entity, after submit, user is
> redirected to entity list with newly created stuff), you may loose the
> message. Another disadvantage of this is non-consistent L&F for feedback
> messages. The advantage is, that you're not covering any content with
> the message as LO does.
>
> * In installation.html I see stuff like <col width="45%">. I think
this
> should be rewritten to use the bootstrap grid layout (classes like
> col-md-12...).
>
> * We use our custom directive lo-autofocus for the 1st input in modals.
> Right after the modal pops-up, user has the focus on the first input and
> pressing enter submits the modal.
>
> * The last one is not a problem at all, I just don't feel right about
> it :) You use $window.location.href instead $location.absUrl(). They
> seem to do the same, but since you don't use any other $window stuff,
> using $location makes more sense to me.
>
>
>
> Btw, we were thinking about creating a dynamic creation of breadcrumbs,
> too. But since we didn't want to have them fully dependent on the URL
> (URLs could change for us in the future), we didn't do it. Are you sure
> you want to make url and breadcrumbs to be linked this way?
>
> Anyway, congrats, you did good job with the console and this review
> helped me to get aware of some problems the LO console may have, too.
>
> Viliam
> _______________________________________________
> aerogear-dev mailing list
> aerogear-dev(a)lists.jboss.org
> https://lists.jboss.org/mailman/listinfo/aerogear-dev
_______________________________________________
aerogear-dev mailing list
aerogear-dev(a)lists.jboss.org
https://lists.jboss.org/mailman/listinfo/aerogear-dev
3617
days inactive
3617
days old
2 comments
3 participants
participants (3)
-
Burr Sutter -
Jay Balunas -
Lukáš Fryč