SUP-2481 intergrate web links api #556
Conversation
-- Added option to add web link in UI. Seems like API don't have support for adding web links as of now.
Conflicts: app/directives/external-account/external-account.directive.js app/services/user.service.js
Merge branch 'feature/sup-1611-web-link-support-ui' into feature/sup-2481-intergrate-web-links-api * feature/sup-1611-web-link-support-ui: more fixes major refactoring, moving data processing to services SUP-1611, Settings: Support adding an external web link. Conflicts: app/directives/challenge-tile/challenge-tile.directive.jade app/directives/external-account/external-account.directive.js app/directives/external-account/external-link-data.directive.jade app/profile/about/about.jade app/settings/edit-profile/edit-profile.controller.js app/specs.html
-- Added more unit tests -- Fixed existing unit tests -- Fixed UI for Adding link
* dev: Update Fix search bar, styling and placement of user handle Move footer links to bottom of mobile nav, style white Fix formatting Fix import adjusted winner ribbon positioning Fix test Remove stopPropagation fixed srm calc fixed some design card display stuff Prevent the section from flickering quick fix more nav styling changes removing "only" from tests and fixing logout redirecting logout to home page, removing cookies etc SUP-2272, Profile--> Remove the mouse hover from profile image. SUP-2420, SiteMap--> Remove the 'Footer' section. Finished nav basics placeholder logo 'beta' tag css fix
-- More unit tests -- Fixed removeLink method in externalLinks.service.js
* dev: (25 commits) removed extra <git> fixed JADE style spacing; css clean, added helper classes for spacings (margin) button spacing fixed form > p style fixed back on track container style added, consistent with everything else. added .csscomb.json; fixed password style fixing the password styles Format and remove console logs iff -> ternary Add styleguide generator Hide money if 0 Fix alignment for srm cards Remove flex on mobile and add vertical align SUP-2577, Bug Hunt should show wins not ratings as the main metric in profile and dashboard SUP-2599, Change copy on empty state for dashboard buncha new tests Change 2 links to use states Add includes and proper variables SUP-2582, Upcoming SRMs should be ordered by most recent date SUP-2582, Upcoming SRMs should be ordered by most recent date ...
-- Removed description field from the card.
-- Ellipsis the text for title field in the data card. Unable to do the same for hyperlink (URL field) because the angular-ellipsis is not working for it.
-- Fixed determination of pending status for an external account. It was using wrong field to set pending status. Came across this issue while debugging for SUP-2479 -- Fixed existing unit tests and added new for pending status fix
| // check if data | ||
| if ($scope.linksData[$scope.accountList[i].provider]) { | ||
| $scope.accountList[i].status = 'linked'; | ||
| if (newValue) { |
There was a problem hiding this comment.
@parthshah please let me know if we really need to compare newValue against oldValue for this check. Earlier, it was doing the check newValue != oldValue which was causing the wrong states of the accounts when we change the values in existing linkedAccount variable. This was also causing unit tests(external-account.directive.spec.js) to not work as expected and I saw some workarounds ($inject and $timeout) there. I have removed those as well.
There was a problem hiding this comment.
In general, it's my understanding that new and old values should always be compared before performing any intensive work to reduce unnecessary updates. Do you think that check needs to be altered since we are watching a collection?
There was a problem hiding this comment.
I think $watchCollection checks one layer deep already by default. So unless we're checking nested elements, we should only see a newValue if Angular detected changes already. Although, we should test this to make sure.
There was a problem hiding this comment.
Agree with Nick, specially when we are watching objects/arrays in comparison to primitive values. Further, the equality operator checks for the references which is always same if alter (push/pop) the existing array.
-- Code review changes
-- Fixed NPE when a new external account is added.
* dev: updating version fix for SUP-2224 Fixed broken tests added extra check addressed comment One more change margin change Update version of ng-iso-constants made changes as per vic Some fixes Added front end sorting for history graph Cleanup Fixed image count SUP-2577, Bug Hunt should show wins not ratings as the main metric in profile and dashboard Fixed track and subtrack ordering SUP-2398, Skill Picker-->'Done' button is getting enabled for fraction of second after clicking on to done the informations. don't open community links in new window
-- More tests for external account directive (now it includes link and unlink method too)
| $scope.accountList = _accountList; | ||
| } else { | ||
| // reset the status for all accounts | ||
| angular.forEach(_accountList, function(account) { |
There was a problem hiding this comment.
@parthshah do you think we should do this? I did that while writing unit tests for the directive. The use case is that whenever we set the linkedAccounts to null values, ideally it should reset the accountList state to indicate that no account is linked yet. Correct? Test is titled with "should reset accountList when linkedAccounts set to null"
There was a problem hiding this comment.
I think this makes sense.
|
lets build |
|
Is this ready to be merged? |
|
It seems now external account synchronizer is working for now links. Thanks @santthosh |
-- Fixed CSS for mobile and desktop view -- Fixed issue where added link was not visible immediately after addition -- Handled already added link error
* dev: generic math icon added small fixes with missing IDs SUP-2729, Publish unit test results with build SUP-2729, Publish unit test results with build SUP-2673, Integrate with TravisCI SUP-2673, Integrate with TravisCI SUP-2673, Integrate with TravisCI skill icons updated removed hover for selected tab on subtrack Updated members of the month Mysterious solution because abs positioning is weird w/ flex cleanup Added reliability link for development Try to gzip css too Add styleguide folder to .gitignore switched to lodash sort Added sorting of submissions by placement SUP-2673, Integrate with TravisCI SUP-2486, Sticky header on profile is not working on Firefox
|
I think now we can merge it. :) |
…-web-links-api SUP-2481 intergrate web links api
|
Merged! |
@parthshah @nlitwin I am initiating the PR so that we get some time to review the changes, considering the fact that it has lot of files changed and major refactoring of the external link components. I have written some new test cases as well to ensure the components are working as expected. As of now, things remaining for the feature to complete are: