-
-
Notifications
You must be signed in to change notification settings - Fork 133
live comments: auto show new comments #2355
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
live comments: auto show new comments #2355
Conversation
…icon, remove new comment IDs per outline removal
…cted comments, preserveScroll utility
…and thread handling; non-critical fix: always give rootLastCommentAt a value
…e center of the viewport; cleanup: add more comments, add cleanup function
…w reference element position
…s presence - de-outlining now happens only for outlined comments - enhanced outlining: add outline only if isNewComment - de-outlining will remove the new comments favicon - on unmount remove the new comments favicon
…nts_enhancements_p2_autoshow
…er animation ends
…ents, remove newComments field
…o comments have been injected - don't preserve scroll if after deduplication we don't inject any comments - use manual read/write cache updates to control the flow -- allows to check if we are really injecting or not - reduce polling to 5 seconds instead of 10 - light cleanup -- removed update cache functions -- added 'injected' to typeDefs (gql consistency)
Refactor: + clearer variables + depth calculation utility function + use destructured Apollo cache + extract item object from item query + skip ignored comment instead of ending the loop CSS: + from-to fadeIn animation keyframes - floatingComments unused class Favicon: + provider exported by default
// directly inject new comments into the cache, preserving scroll position | ||
// quirk: scroll is preserved even if we are not injecting new comments due to dedupe | ||
preserveScroll(() => cacheNewComments(cache, rootId, data.newComments.comments, sort)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Scroll is preserved even for comments that ultimately gets deduplicated (e.g. your own comments on the same tab). It may be an inconvenience, not really expensive but it is something to think about.
The most straightforward solution is to:
- Read the existing comments from cache, dedupe, and then maybe write the fragment
But I realized that writeFragment
always returns the Item reference ID in the cache, even when it fails. This messes up things for comments that require different fragments because of the comments
field absence.
Therefore a truly effective solution would be to:
- Read, dedupe, call updateFragment which runs another read and ultimately a write.
updateFragment
will explicitly fail if the provided object doesn't match the fragment, which enables us to retry with other fragments until it fits. But this would mean doing read -> read/write
In addition, since dedupe happens by comparing the new comment with the parent's existing comments, both solutions would preserve scroll for each comment in the newComments
array, spawning several MutationObserver
and requestAnimationFrame
At the actual state, scroll is instead preserved for batches of new comments, but preserveScroll
will run even if no comments are being effectively injected.
const unsetOutline = () => { | ||
if (!ref.current) return | ||
const hasOutline = ref.current.classList.contains('outline-new-comment') || ref.current.classList.contains('outline-new-injected-comment') | ||
const hasOutlineUnset = ref.current.classList.contains('outline-new-comment-unset') | ||
|
||
// don't try to unset the outline if the comment is not outlined or we already unset the outline | ||
if (hasOutline && !hasOutlineUnset) { | ||
ref.current.classList.add('outline-new-comment-unset') | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Outlining is addressed by #2401 with a separate hook, as it's becoming too big to stay here
export function calculateDepth (path, rootId, parentId) { | ||
// calculate depth by counting path segments from root to parent | ||
const pathSegments = path.split('.') | ||
const rootIndex = pathSegments.indexOf(rootId.toString()) | ||
const parentIndex = pathSegments.indexOf(parentId.toString()) | ||
|
||
// depth is the distance from root to parent in the path | ||
const depth = parentIndex - rootIndex | ||
|
||
return depth | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is in place to avoid injecting comments that are not visible by the user.
We did this to show accurate counts and avoid misleading the user, but since we automatically inject now this is not required anymore.
Removing depth protection will update comments that are not visible but present in cache, useful to show that new comments are present down the tree, especially with #2401 that will outline the MoreReplies
button.
But, there might be comments that are not present in cache, like when we fully reload an Item and it loads them until the depth limit has been reached. In this case new comments to these absent comments are dropped.
…deps from outlining
Description
Closes #2358
Auto-shows new comments as they arrive, with a neat CSS fade-in animation, preserving the scroll position on injection by making use of
MutationObserver
to detect DOM mutations andrequestAnimationFrame
to update the scroll position before the next repaint.Also introduces a slightly tweaked outlining system, that de-outlines the comment only if it actually has an outline/hasn't been already de-outlined.
Legacy live comments code has been deleted.
Screenshots
Auto-show animation
autoshow-base.mp4
Scroll position is preserved to avoid layout shifts
autoshow-preservescroll.mp4
Additional Context
Preserve scroll pipeline
window.scrollY
)ref
) at the center of the viewportMutationObserver
watches the page for DOM changesWhen a DOM mutation fires:
refMoved > 0
then something was inserted above the anchor, pushing it downwards.refMoved
amountThe MutationObserver is disconnected at the end of the scroll changes or after a 1 second fallback timeout in the case of no DOM mutations (although almost impossible)
List of changes
Quirks
Checklist
Are your changes backward compatible? Please answer below:
For example, a change is not backward compatible if you removed a GraphQL field or dropped a database column.
Yes
On a scale of 1-10 how well and how have you QA'd this change and any features it might affect? Please answer below:
7
For frontend changes: Tested on mobile, light and dark mode? Please answer below:
n/a
Did you introduce any new environment variables? If so, call them out explicitly here:
n/a