Skip to content
This repository was archived by the owner on Mar 4, 2025. It is now read-only.

Feature/sup 2150 ext acc card add profile url link #601

Merged
merged 3 commits into from
Dec 10, 2015
Merged

Feature/sup 2150 ext acc card add profile url link #601

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
e52c2eb
SUP-2150, Clicking on external account cards (profile) does nothing
Dec 9, 2015
8a64b97
SUP-2788, Weblinks-- Mousehover on the whole card, but clicking on ca…
Dec 9, 2015
73ea575
SUP-2150, Clicking on external account cards (profile) does nothing
Dec 10, 2015
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions app/directives/external-account/external-link-data.directive.jade
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
.external-link-list
div.external-link-tile(ng-repeat="account in linkedAccountsData", ng-class="{'external-link-tile--editable' : editable}")
div.external-link-tile(ng-repeat="account in linkedAccountsData", ng-class="{'external-link-tile--editable' : editable, 'external-link-tile--pending' : account.status === 'PENDING' || account.data.status === 'PENDING'}", ng-click="openLink(account)")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why do we need to check both account.status and account.data.status? is the API not consistent?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

instead of using a div would it help to use anchor tag here? We wouldn't need an anchor tag for the url in that case..

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess that depends on whether we want the whole thing clickable or just the anchor tag below. But we can delete div since we have an .external-link-tile class :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We are maintaining different nesting for added external account and web link. I was thinking to clean it up, however, it would have required more time and testing. Along with that, I was not sure which data structure to cleanup. For external accounts, we are nesting the added external account one more deep level e.g.

{
    status: 'SUCCESS' | 'ERROR',
    linkedAccount: {
        provider: 'github',
        data: {
            handle: '',
            status: 'PENDING',
            ...
        }
    }
}

while for external web links, it is

{
    provider: 'weblink',
    data: {
        handle: '',
        url: '',
        title: '',
        status: 'PENDING'
    }
}

Can you please suggest which one to fix?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The rationale behind not using anchor tag is that it defeats the purpose of the anchor tag. I think it is not recommended to render a complicated HTML inside an anchor tag. Regarding the anchor tag on URL, we can still avoid it because if I remove the preventEventPropagation directive from it, it would bubble the event to the div and it would take user to the link. Only thing would be that we have to style the URL text as anchor.

.top
.ext-link-tile_edit-header(ng-show="editable && account.provider === 'weblink'")
.ext-link-tile_edit-header_delete(ng-click="confirmDeletion(account)", ng-class="{'ext-link-tile_edit-header_delete--disabled': account.deletingAccount || account.status === 'PENDING'}")
Expand Down Expand Up @@ -106,4 +106,4 @@
p.link-title(data-ellipsis, ng-bind="account.title", ng-hide="account.status === 'PENDING'")
p.link-title(ng-show="account.status === 'PENDING'") Loading data. This will take a few minutes.

a.link-url(ng-href="{{account.URL | urlProtocol}}", ng-bind="account.URL")
a.link-url(ng-href="{{account.URL | urlProtocol}}", ng-bind="account.URL", target="_blank",prevent-event-propagation)
19 changes: 17 additions & 2 deletions app/directives/external-account/external-links-data.directive.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,12 +18,27 @@
editable: '=',
userHandle: '@'
},
controller: ['$log', '$scope', 'ExternalWebLinksService', 'toaster', 'ngDialog',
function($log, $scope, ExternalWebLinksService, toaster, ngDialog) {
controller: ['$log', '$scope', '$window', '$filter', 'ExternalWebLinksService', 'toaster', 'ngDialog',
function($log, $scope, $window, $filter, ExternalWebLinksService, toaster, ngDialog) {

$log = $log.getInstance("ExternalLinksDataCtrl");
$scope.deletionDialog = null;

$scope.openLink = function(account) {
var url = null;
if (account) {
if (account.data && account.data.profileURL && account.data.status !== 'PENDING') {
url = account.data.profileURL;
}
if (account.URL && account.status !== 'PENDING') {
url = account.URL;
}
}
if (url) {
$window.open($filter('urlProtocol')(url), '_blank');
}
}

$scope.confirmDeletion = function(account) {
$scope.deletionDialog = ngDialog.open({
className: 'ngdialog-theme-default tc-dialog',
Expand Down
15 changes: 15 additions & 0 deletions app/directives/preventEventPropagation.directive.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
(function() {
'use strict';

angular.module('tcUIComponents').directive('preventEventPropagation', preventEventPropagation);

preventEventPropagation.$inject = ['$timeout', '$parse'];

function preventEventPropagation($timeout, $parse) {
return function(scope, element, attr) {
element.bind('click', function(evt) {
evt.stopPropagation();
});
};
}
})();
1 change: 1 addition & 0 deletions app/index.jade
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -205,6 +205,7 @@ html
script(src="directives/on-file-change.directive.js")
script(src="directives/onoffswitch/onoffswitch.directive.js")
script(src="directives/page-state-header/page-state-header.directive.js")
script(src="directives/preventEventPropagation.directive.js")
script(src="directives/profile-widget/profile-widget.directive.js")
script(src="directives/responsive-carousel/responsive-carousel.directive.js")
script(src="directives/skill-tile/skill-tile.directive.js")
Expand Down
6 changes: 6 additions & 0 deletions assets/css/directives/external-link-data.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -313,5 +313,11 @@ external-accounts {
.external-link-tile--editable {
// height: 285px;
}

.external-link-tile--pending {
&:hover {
cursor: default;
}
}
}
}