[aerogear-dev] AG Push Console review

[Jay Balunas]

On May 29, 2014, at 12:36 PM, Burr Sutter <bsutter at 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 at 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 at redhat.com>
>> To:	Matthias Wessendorf <mwessend at redhat.com>, Lukas Fryc <lfryc at redhat.com>
>> CC:	Alexandre Mendonca <amendonc at 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 at lists.jboss.org
>> https://lists.jboss.org/mailman/listinfo/aerogear-dev
> 
> _______________________________________________
> aerogear-dev mailing list
> aerogear-dev at lists.jboss.org
> https://lists.jboss.org/mailman/listinfo/aerogear-dev

-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://lists.jboss.org/pipermail/aerogear-dev/attachments/20140529/f661e49e/attachment-0001.html