launchpad-reviewers team mailing list archive

[Ines Almeida <mp+458730@xxxxxxxxxxxxxxxxxx>]

Diff comments:

> diff --git a/lib/lp/app/browser/tales.py b/lib/lp/app/browser/tales.py
> index eb5cad2..9ec03a3 100644
> --- a/lib/lp/app/browser/tales.py
> +++ b/lib/lp/app/browser/tales.py
> @@ -3049,3 +3050,32 @@ class IRCNicknameFormatterAPI(ObjectFormatterAPI):
>              self._context.nickname,
>              self._context.network,
>          ).escapedtext
> +
> +
> +@implementer(ITraversable)
> +class SocialAccountFormatterAPI(ObjectFormatterAPI):
> +    """Adapter from social account objects to a formatted string."""
> +
> +    traversable_names = {
> +        "formatted_display": "formatted_display",
> +    }
> +
> +    def getPlatformClass(self):
> +        return SOCIAL_PLATFORM_TYPES_MAP.get(self._context.platform)
> +
> +    def icon(self, platform):
> +        return (
> +            f'<img class="social_accounts__icon" alt="{platform.title}" '
> +            f'title="{platform.title}" src="/@@/{platform.icon}" />'
> +        )
> +
> +    def formatted_display(self, view_name=None):

I don't really agree:
 - Only the IRC icon isn't standard, the others are
 - The icons have tooltips when hovered, which is quite standard
 - No one uses Launchpad on their phones for the lack of tooltip on mobile to be an issue
 - Except for IRC and Jabber, most social platforms we will add (including matrix) will have a link to the platform
 - People will get used to the icons
 - Adding the names of the platforms will look a lot worse style-wise

> +        platform = self.getPlatformClass()
> +        icon = self.icon(platform)
> +        text_display = platform.display_format.format(**self._context.identity)
> +
> +        if platform.url:
> +            url = platform.url.format(**self._context.identity)
> +            text_display = f'<a href={url} target="_blank">{text_display}</a>'
> +
> +        return structured(f"{icon} {text_display}").escapedtext


-- 
https://code.launchpad.net/~ines-almeida/launchpad/+git/launchpad/+merge/458730
Your team Launchpad code reviewers is requested to review the proposed merge of ~ines-almeida/launchpad:social-accounts-display-view into launchpad:master.