Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Direct reacting from notifications #6935

Merged
merged 4 commits into from
Aug 28, 2021
Merged

Conversation

rafael-xmr
Copy link
Collaborator

@rafael-xmr rafael-xmr commented Aug 23, 2021

Fixes

Issue Number: #5062, and closes #6906

(Previously #6711, now with issues addressed)

What is the new behavior?

Peek 2021-08-23 11-57

PR Type & Checklist

PR Type...

What kind of change does this PR introduce?

  • Bugfix
  • Feature
  • Code style update (formatting)
  • Refactoring (no functional changes)
  • Documentation changes
  • Other - Please describe:
PR Checklist...

Please check all that apply to this PR using "x":

  • I have checked that this PR is not a duplicate of an existing PR (open, closed or merged)
  • I added a line describing my change to CHANGELOG.md
  • I have checked that this PR does not introduce a breaking change
  • This PR introduces breaking changes and I have provided a detailed explanation below

Copy link
Contributor

@infinite-persistence infinite-persistence left a comment

Choose a reason for hiding this comment

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

Good approach in general, touching minimal files as possible.

A few issues found; mainly concerned with the lag. Not sure if it's due to the excessive API calls (mentioned below), or perhaps unintended re-renders.

ui/scss/component/_notification.scss Outdated Show resolved Hide resolved
ui/page/notifications/view.jsx Show resolved Hide resolved
ui/scss/component/_notification.scss Outdated Show resolved Hide resolved
ui/component/notification/view.jsx Outdated Show resolved Hide resolved
Comment on lines +215 to +183
<div
className={classnames('notification__wrapper', {
'notification__wrapper--unread': !is_read,
})}
>
<Wrapper>
Copy link
Contributor

@infinite-persistence infinite-persistence Aug 24, 2021

Choose a reason for hiding this comment

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

Lots of console errors

Copy link
Contributor

Choose a reason for hiding this comment

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

This one still exists. Can ship, although we should stop introducing console errors to a already-long list (e.g. Modal dismissal, Reach, videojs)

ui/component/notification/view.jsx Outdated Show resolved Hide resolved
@@ -11,6 +11,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/).
- Add confirmation on comment removal _community pr!_ ([#6563](https://github.com/lbryio/lbry-desktop/pull/6563))
- Show on content page if a file is part of a playlist already _community pr!_([#6393](https://github.com/lbryio/lbry-desktop/pull/6393))
- Add filtering to playlists ([#6905](https://github.com/lbryio/lbry-desktop/pull/6905))
- Added direct replying to notifications _community pr!_ ([#6935](https://github.com/lbryio/lbry-desktop/pull/6935))
Copy link
Contributor

@infinite-persistence infinite-persistence Aug 24, 2021

Choose a reason for hiding this comment

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

File thumbnail flickers a lot

Probably a non-blocker, but a bit annoying. Was present in the past, but now super obvious.

It flickers when:

  • you refresh the Notifications Page (or initial load)
  • On a comment notification, click "Reply" to expand the text box (notice other notifications with file thumbnails will flicker).
  • On a comment notification, click "Cancel" to close the text box (notice other notifications with file thumbnails will flicker).

Copy link
Contributor

@infinite-persistence infinite-persistence Aug 24, 2021

Choose a reason for hiding this comment

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

[Non-blocker] Unknown error

Not sure how to reproduce this. Happens only on a few notifications when replying.

Copy link
Contributor

@infinite-persistence infinite-persistence Aug 24, 2021

Choose a reason for hiding this comment

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

Ah, it happens when the actual comment has been deleted. Not sure how to handle this gracefully. Probably can ignore for now, unless you have some ideas.

Copy link
Collaborator Author

@rafael-xmr rafael-xmr Aug 27, 2021

Choose a reason for hiding this comment

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

challenge accepted

Peek 2021-08-27 09-11

Copy link
Member

Choose a reason for hiding this comment

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

@infinite-persistence if this looks good on another review, please merge

ui/scss/component/_notification.scss Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Notifications: Separator lines missing
3 participants