@julienmoumne opened this Pull Request on June 19th 2014 Member
@mattab commented on June 20th 2014 Member

That's an epic pull request.... well done Julien!

  • in the build there's an integration test file missing: test_apiGetReportMetadata__API.getIpFromHeader.xml which you can generate with: phpunit Integration/ApiGetReportMetadataTest.php
  • Ping @tsteur our resident AngularJS expert. Would you mind helping Julien with code review?
@mattab commented on June 25th 2014 Member

Feedback:

  • on hover on a row, show cursor:pointer
  • on click on a row, it should edit the clicked website
  • when 'Edit' or 'Delete' links are hovered, put them underlined
  • when a website is ecommerce enabled, in the table, Expected: Yes, Got: -
  • to save some horizontal space, the column heading Excluded Parameters should be Excluded<br>Parameters
  • when websites are loading in the table, display "Loading..."
  • the tooltip Enter a comma separated list of all query parameter names containing the site search keyword. should be displayed on same level as its input field Query parameter. Got: inline help displayed below input field.
  • same with Category parameter inline help
  • when clicking Add a new website the screen should scroll to the row for the new website (maybe missing piwikHelper.lazyScrollTo?)
  • when deleting a website, in the popover, Expected buttons: Yes / No, Got: Cancel / Ok
  • The timezone selector is missing the timezone groups (displayed in bold)

When a website is being edited:

  • or Cancel should be black color, on a new line (got: blue color, on same line as "Save" button")
  • Save button should have a margin-top:50px or similar (got: one line break after website name input field)
  • Pressing Enter key, should be same as clicking Save button (maybe submitSiteOnEnter missing?)

Minor code cosmetics

  • single statement should have braces { } (eg. in API.php)
  • comaDelimitedFieldToArray should be commaDelimitedFieldToArray
  • maybe you could ask Thomas his opinion about places tagged // can probably be shared

Very clean code... it will for sure serve as inspiration for our future AngularJS work. Well done mate!

@mattab commented on July 2nd 2014 Member

Well done @JulienMoumne this is impressive work!

It's very nice to see you leading the way in our AngularJS migration.

@tsteur commented on July 2nd 2014 Member

Looking forward to seeing more of this @JulienMoumne :)

This Pull Request was closed on July 2nd 2014
Powered by GitHub Issue Mirror