[aerogear-dev] AG Push Console review

[Burr Sutter]

This is great feedback - thank you Viliam


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

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