Uploaded image for project: 'AeroGear'
  1. AeroGear
  2. AEROGEAR-6945

Admin UI: apply findings of Code Style review

    XMLWordPrintable

Details

    • Enhancement
    • Resolution: Done
    • Major
    • None
    • None
    • None

    Description

      Forum discussion:
      http://aerogear-dev.1069024.n5.nabble.com/aerogear-dev-AG-Push-Console-review-td8004.html


      The name of your angularjs module is "newadminApp". I think it should
      be something like "agconsole".

      created AGPUSH-719

      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.

      created AGPUSH-720

      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.

      created AGPUSH-721

      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.

      I don't consider this bad practice since it explains well that some options has same value as given key:
      https://github.com/aerogear/aerogear-unifiedpush-server/blob/f2c5a5d7d834cc74e4a66bcb966437d2de68981d/admin-ui/app/scripts/controllers/DetailController.js#L186

      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.

      created AGPUSH-722

      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.

      created AGPUSH-723

      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.

      Resolved by AGPUSH-659

      Attachments

        Activity

          People

            Unassigned Unassigned
            lfryc Lukáš Fryč (Inactive)
            Votes:
            0 Vote for this issue
            Watchers:
            1 Start watching this issue

            Dates

              Created:
              Updated:
              Resolved: