LBRYCommunity/lbry-desktop
1
0
Fork 0
Code Issues 596 Pull requests 6 Projects Releases 238 Packages Wiki Activity

Notifications improve links #6711

Merged
saltrafael merged 1 commit from notifications_click into master 2021-08-18 23:40:36 +02:00
Conversation 6 Commits 1 Files changed 3 +81 -34
3 changed files with 81 additions and 34 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

1
CHANGELOG.md
View file

@ -19,6 +19,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/).
- Update lighthouse search api _community pr!_ ([#6731](https://github.com/lbryio/lbry-desktop/pull/6731))
- Update sockety api _community pr!_ ([#6747](https://github.com/lbryio/lbry-desktop/pull/6747))
- Use resolve for OG metadata instead of chainquery _community pr!_ ([#6787](https://github.com/lbryio/lbry-desktop/pull/6787))
- Improved clickability of notification links _community pr!_ ([#6711](https://github.com/lbryio/lbry-desktop/pull/6711))
### Fixed
- App now supports '#' and ':' for claimId separator ([#6496](https://github.com/lbryio/lbry-desktop/pull/6496))

110
ui/component/notification/view.jsx
View file

@ -16,6 +16,8 @@ import FileThumbnail from 'component/fileThumbnail';
import { Menu, MenuList, MenuButton, MenuItem } from '@reach/menu-button';
import NotificationContentChannelMenu from 'component/notificationContentChannelMenu';
import LbcMessage from 'component/common/lbc-message';
import UriIndicator from 'component/uriIndicator';
import { NavLink } from 'react-router-dom';
type Props = {
notification: WebNotification,
@ -29,13 +31,12 @@ export default function Notification(props: Props) {
const { notification, menuButton = false, doReadNotifications, doDeleteNotification } = props;
const { push } = useHistory();
const { notification_rule, notification_parameters, is_read, id } = notification;
const isCommentNotification =
notification_rule === RULE.COMMENT ||
notification_rule === RULE.COMMENT_REPLY ||
notification_rule === RULE.CREATOR_COMMENT;
const commentText = isCommentNotification && notification_parameters.dynamic.comment;
const channelUrl =
(notification_rule === RULE.NEW_CONTENT && notification.notification_parameters.dynamic.channel_url) || '';
let notificationTarget;
switch (notification_rule) {
@ -51,21 +52,14 @@ export default function Notification(props: Props) {
notificationTarget = notification_parameters.device.target;
}
let notificationLink = formatLbryUrlForWeb(notificationTarget);
let urlParams = new URLSearchParams();
if (isCommentNotification && notification_parameters.dynamic.hash) {
urlParams.append('lc', notification_parameters.dynamic.hash);
}
try {
const { isChannel } = parseURI(notificationTarget);
if (isChannel) {
urlParams.append(PAGE_VIEW_QUERY, DISCUSSION_PAGE);
}
} catch (e) {}
notificationLink += `?${urlParams.toString()}`;
const creatorIcon = (channelUrl) => {
return (
infinite-persistence commented 2021-08-03 04:51:45 +02:00 (Migrated from github.com)
Review
Copy link
  • This ends up in an infinite loop of reaction.List calls (see Network Tab in Dev Tools). The backend team will scold us again, lol.

Suggestions:

  • We only need to fetch the reactions 1 time per mount. Using a blank [] dependency list on the useEffect would work, assuming we have all the variables that we need on mount.
  • However, after talking to Tom, we think it is highly-likely that popular creators will have lots of comment notifications. So, fetching it at the Notification level will create lots of reaction.List calls (1 for each Notification). It will be more efficient to do it at NotificationsPage, since we have the full list comments to fetch for already. But we leave this up to you -- we can also make the change to NotificationsPage at a later time when we see huge spikes in reaction.List calls.
- [ ] This ends up in an infinite loop of `reaction.List` calls (see Network Tab in Dev Tools). The backend team will scold us again, lol. Suggestions: - We only need to fetch the reactions 1 time per mount. Using a blank `[]` dependency list on the `useEffect` would work, assuming we have all the variables that we need on mount. - However, after talking to Tom, we think it is highly-likely that popular creators will have lots of comment notifications. So, fetching it at the `Notification` level will create lots of `reaction.List` calls (1 for each `Notification`). It will be more efficient to do it at `NotificationsPage`, since we have the full list comments to fetch for already. But we leave this up to you -- we can also make the change to `NotificationsPage` at a later time when we see huge spikes in `reaction.List` calls.
<UriIndicator uri={channelUrl} link>
<ChannelThumbnail small uri={channelUrl} />
</UriIndicator>
);
};
infinite-persistence commented 2021-07-29 07:57:42 +02:00 (Migrated from github.com)
Review
Copy link

This one is probably not robust as it relies on how Commentron phrases the suggested title.
(e.g. will fail if phrasing changed to "@@Welcome to my post@@" received a new comment from @mike).

It might be better to inspect notification_parameters.dynamic for the author, but you'll need to do it differently for each notification type that has a channel-mention. For example, the author for a comment-notification can be parsed from notification_parameters.dynamic.comment_author.

Aside: It is also possible to reconstruct the title ourselve from notification_parameters's info. device.title is just Commentron's default.

This one is probably not robust as it relies on how Commentron phrases the suggested title. (e.g. will fail if phrasing changed to `"@@Welcome to my post@@" received a new comment from @mike`). It might be better to inspect `notification_parameters.dynamic` for the author, but you'll need to do it differently for each notification type that has a channel-mention. For example, the author for a comment-notification can be parsed from `notification_parameters.dynamic.comment_author`. Aside: It is also possible to reconstruct the title ourselve from `notification_parameters`'s info. `device.title` is just Commentron's default.
infinite-persistence commented 2021-07-29 08:20:27 +02:00 (Migrated from github.com)
Review
Copy link

@jessopb, I believe this block is trying replace the channel name with something that's clickable. I recall this issue also affects livestream chat and maybe other places. Do you have a recommendation on how to address this issue in general?

The above breaks up the string. This will break any hopes of localizing the notifications page in the future.

Maybe something like how LbcMessage does the replacement will be a cleaner approach?

But anyway, since Commentron doesn't support a i18n-friendly format yet, this solution is still better than current state.

@jessopb, I believe this block is trying replace the channel name with something that's clickable. I recall this issue also affects livestream chat and maybe other places. Do you have a recommendation on how to address this issue in general? The above breaks up the string. This will break any hopes of localizing the notifications page in the future. - <img src="https://user-images.githubusercontent.com/64950861/127441224-22c8ef63-71e6-4938-8c3e-fd4692bae5a4.png" width="400"> - <img src="https://user-images.githubusercontent.com/64950861/127441451-c35e5f70-8657-4196-b502-9948032f7d01.png" width="400"> Maybe something like how `LbcMessage` does the replacement will be a cleaner approach? But anyway, since Commentron doesn't support a i18n-friendly format yet, this solution is still better than current state.
infinite-persistence commented 2021-07-29 08:44:48 +02:00 (Migrated from github.com)
Review
Copy link

Or maybe something like this, which also fixes the i18n problem:

  const commentedOnClaimMsg = (
    <I18nMessage
      tokens={{
        comment_author: <UriIndicator uri={notification_parameters.dynamic.comment_author} link />,
        claim_title: notification_parameters.dynamic.claim_title,
      }}
    >
      %comment_author% commented on %claim_title%
    </I18nMessage>
  );
Or maybe something like this, which also fixes the i18n problem: ``` const commentedOnClaimMsg = ( <I18nMessage tokens={{ comment_author: <UriIndicator uri={notification_parameters.dynamic.comment_author} link />, claim_title: notification_parameters.dynamic.claim_title, }} > %comment_author% commented on %claim_title% </I18nMessage> ); ```
let channelUrl;
let icon;
switch (notification_rule) {
case RULE.CREATOR_SUBSCRIBER:
@ -73,16 +67,20 @@ export default function Notification(props: Props) {
break;
case RULE.COMMENT:
case RULE.CREATOR_COMMENT:
icon = <ChannelThumbnail small uri={notification_parameters.dynamic.comment_author} />;
channelUrl = notification_parameters.dynamic.comment_author;
icon = creatorIcon(channelUrl);
break;
case RULE.COMMENT_REPLY:
icon = <ChannelThumbnail small uri={notification_parameters.dynamic.reply_author} />;
channelUrl = notification_parameters.dynamic.reply_author;
icon = creatorIcon(channelUrl);
break;
case RULE.NEW_CONTENT:
icon = <ChannelThumbnail small uri={notification_parameters.dynamic.channel_url} />;
channelUrl = notification_parameters.dynamic.channel_url;
icon = creatorIcon(channelUrl);
break;
case RULE.NEW_LIVESTREAM:
icon = <ChannelThumbnail small uri={notification_parameters.dynamic.channel_url} />;
channelUrl = notification_parameters.dynamic.channel_url;
icon = creatorIcon(channelUrl);
break;
case RULE.DAILY_WATCH_AVAILABLE:
case RULE.DAILY_WATCH_REMIND:
@ -97,12 +95,54 @@ export default function Notification(props: Props) {
icon = <Icon icon={ICONS.NOTIFICATION} sectionIcon />;
}
let notificationLink = formatLbryUrlForWeb(notificationTarget);
let urlParams = new URLSearchParams();
if (isCommentNotification && notification_parameters.dynamic.hash) {
urlParams.append('lc', notification_parameters.dynamic.hash);
}
let channelName = channelUrl && '@' + channelUrl.split('@')[1].split('#')[0];
const notificationTitle = notification_parameters.device.title;
infinite-persistence commented 2021-08-03 06:07:57 +02:00 (Migrated from github.com)
Review
Copy link

Feels like can be improved, but will pass for now so as not to stall the PR further.

With a little more work, I think we can avoid the unnecessary loop and also enable localization at the same time using something like the following (incomplete example). It'll also prevent the need to put a blanket LbcMessage for notifications that don't need it. We can also customize the phrase like adding quotes to the title for readability.

function generateTitle(rule, params) {
    switch (rule) {
      case RULE.COMMENT:
        return (
          <I18nMessage
            tokens={{
              commenter: <UriIndicator uri={notification_parameters.dynamic.comment_author} link />,
              title: notification_parameters.dynamic.claim_title,
            }}
          >
            %commenter% commented on "%title%"
          </I18nMessage>
        );

      case RULE.CREATOR_SUBSCRIBER:
        // TODO
        break;
      case RULE.CREATOR_COMMENT:
        // TODO
        break;
      case RULE.COMMENT_REPLY:
        // TODO
        break;
      case RULE.NEW_CONTENT:
      case RULE.NEW_LIVESTREAM:
      case RULE.DAILY_WATCH_AVAILABLE:
      case RULE.DAILY_WATCH_REMIND:
      case RULE.MISSED_OUT:
      case RULE.REWARDS_APPROVAL_PROMPT:
      case RULE.FIAT_TIP:
      default:
        // Use Commentron default
        return params.device.title;
    }
  }

  const title = generateTitle(notification_rule, notification_parameters);
Feels like can be improved, but will pass for now so as not to stall the PR further. With a little more work, I think we can avoid the unnecessary loop and also enable localization at the same time using something like the following (incomplete example). It'll also prevent the need to put a blanket `LbcMessage` for notifications that don't need it. We can also customize the phrase like adding quotes to the title for readability. ``` function generateTitle(rule, params) { switch (rule) { case RULE.COMMENT: return ( <I18nMessage tokens={{ commenter: <UriIndicator uri={notification_parameters.dynamic.comment_author} link />, title: notification_parameters.dynamic.claim_title, }} > %commenter% commented on "%title%" </I18nMessage> ); case RULE.CREATOR_SUBSCRIBER: // TODO break; case RULE.CREATOR_COMMENT: // TODO break; case RULE.COMMENT_REPLY: // TODO break; case RULE.NEW_CONTENT: case RULE.NEW_LIVESTREAM: case RULE.DAILY_WATCH_AVAILABLE: case RULE.DAILY_WATCH_REMIND: case RULE.MISSED_OUT: case RULE.REWARDS_APPROVAL_PROMPT: case RULE.FIAT_TIP: default: // Use Commentron default return params.device.title; } } const title = generateTitle(notification_rule, notification_parameters); ```
const titleSplit = notificationTitle.split(' ');
let fullTitle = [' '];
let uriIndicator;
const title = titleSplit.map((message, index) => {
if (channelName === message) {
uriIndicator = <UriIndicator uri={channelUrl} link />;
fullTitle.push(' ');
const resultTitle = fullTitle;
fullTitle = [' '];
return [resultTitle.join(' '), uriIndicator];
} else {
fullTitle.push(message);
if (index === titleSplit.length - 1) {
return <LbcMessage>{fullTitle.join(' ')}</LbcMessage>;
}
}
});
try {
const { isChannel } = parseURI(notificationTarget);
if (isChannel) {
urlParams.append(PAGE_VIEW_QUERY, DISCUSSION_PAGE);
}
} catch (e) {}
notificationLink += `?${urlParams.toString()}`;
const navLinkProps = {
to: notificationLink,
onClick: (e) => e.stopPropagation(),
};
function handleNotificationClick() {
if (!is_read) {
doReadNotifications([id]);
}
if (notificationLink) {
if (menuButton && notificationLink) {
push(notificationLink);
}
}
@ -120,9 +160,11 @@ export default function Notification(props: Props) {
)
: notificationLink
? (props: { children: any }) => (
<a className="menu__link--notification" onClick={handleNotificationClick}>
{props.children}
</a>
<NavLink {...navLinkProps}>
<a className="menu__link--notification" onClick={handleNotificationClick}>
{props.children}
</a>
</NavLink>
)
: (props: { children: any }) => (
<span
@ -145,17 +187,11 @@ export default function Notification(props: Props) {
<div className="notification__content-wrapper">
<div className="notification__content">
<div className="notification__text-wrapper">
{!isCommentNotification && (
<div className="notification__title">
<LbcMessage>{notification_parameters.device.title}</LbcMessage>
</div>
)}
{!isCommentNotification && <div className="notification__title">{title}</div>}
{isCommentNotification && commentText ? (
<>
<div className="notification__title">
<LbcMessage>{notification_parameters.device.title}</LbcMessage>
</div>
<div className="notification__title">{title}</div>
<div title={commentText} className="notification__text mobile-hidden">
{commentText}
</div>
@ -193,7 +229,13 @@ export default function Notification(props: Props) {
<div className="notification__menu">
<Menu>
<MenuButton className={'menu__button notification__menu-button'} onClick={(e) => e.stopPropagation()}>
<MenuButton
className={'menu__button notification__menu-button'}
onClick={(e) => {
e.preventDefault();
e.stopPropagation();
}}
>
<Icon size={18} icon={ICONS.MORE_VERTICAL} />
</MenuButton>
<MenuList className="menu__list">

4
ui/scss/component/_notification.scss
View file

@ -42,6 +42,7 @@ $contentMaxWidth: 60rem;
width: 100%;
display: flex;
padding: var(--spacing-m) 0;
justify-content: space-between;
.channel-thumbnail {
@include handleChannelGif(3rem);
@ -60,6 +61,7 @@ $contentMaxWidth: 60rem;
.notification__wrapper--unread {
background-color: var(--color-card-background-highlighted);
justify-content: space-between;
&:hover {
background-color: var(--color-button-secondary-bg);
@ -120,6 +122,7 @@ $contentMaxWidth: 60rem;
}
.notification__title {
position: relative;
font-size: var(--font-small);
color: var(--color-text);
margin-bottom: var(--spacing-s);
@ -135,6 +138,7 @@ $contentMaxWidth: 60rem;
.notification__text {
font-size: var(--font-body);
color: var(--color-text);
display: -webkit-box;
-webkit-line-clamp: 1;
-webkit-box-orient: vertical;