Comments improvements - #2476
Merged
Merged
Conversation
Slate splits a decorated range wherever another decoration overlaps it, and every piece carries the range's key - so a range that another thread overlaps registers several anchor elements under one key. The registry held one element per key and deleted the key outright when an element went away, which let a piece unmounting take down the anchor its siblings still provided. The badge then had no reference to position against and rendered nothing. Revealing a resolved thread and hiding it again is enough to trigger it: the highlight comes and goes, the leaves around it are split and merged, and one of the overlapped threads loses its badge until something else makes its leaf re-register. The key now holds every element registered under it, and the badge anchors to whichever comes first in the document - where the range starts, rather than wherever the last piece to mount happened to be.
Two words for the same thing had grown up side by side: the badges and the entry list's dot speak of unread comments, the markers inside a thread of new ones. The activity feed puts them in one view, where "new" reads as recently written - which is what the feed's rows are about, while the markers are about what the reviewer has read. The two counts part ways as soon as a thread was read halfway, so the words have to as well. Everything that means "not read yet" is unread now, down to the summary behind the entry list's dot: #new? becomes #unread?, and its counts follow.
The chevron sat in the thread's corner, far from the replies it hid and in the way of everything else that wants that corner. The count of the replies now carries it: clicking the count no longer makes it disappear into what it opened, it turns the chevron around instead, so the same control closes the thread again. The faces of who replied still go once the replies they stand in for are on screen. A thread alone in its list used to be exempt from collapsing, which left its count line as a control that did nothing. It is picked as the initially expanded thread instead, so the same line closes it again. Lists with more than one thread still open with all of them summarized, and the entry-wide list keeps every thread that way: it renders a list per subject, where one expanded thread each adds up. Freeing the corner lets the comments' menus have it, lined up in a column at the thread's right edge.
Clients read position as meaning: the first comment of a thread is the topic and the rest are replies. Nothing guaranteed that order so far - it held because InnoDB returns rows of a secondary index in primary key order, not because the query asked for it. No behavior change on the database Pageflow runs on, so the spec is a regression guard rather than a failing test made to pass.
The thread JSON starts carrying resolvedById and resolverName, so that clients can say who resolved a thread rather than only when it happened. Both attributes already existed on the model; only the JSON left them out.
Moves formatDate and formatDateTime out of Comment.js so that other review components can render timestamps the same way.
The resolve button says a thread is resolved, but not by whom. Threads carry the resolver since the review index started naming them, so every list that shows a thread can now close it off with a line of its own - the popover and the editor's lists included, which had no way of telling who ended a discussion.
Answers what happened since the reviewer last looked, which neither the structural list nor the toolbar arrows can: both are ordered by document position, not by time. One entry per thread, ordered by the thread's most recent event. A thread rather than an event is the unit because both kinds of context a reviewer needs - the content a comment is about and the discussion around it - belong to the thread; per event, a thread that gained four replies would repeat itself four times over. Where a thread sits in the entry is left to the thread as well, so the entry carries it rather than a description of it. Lifts the unread predicate out of unreadComments so that the feed marks entries by the same rule - baseline included - rather than restating it, and so that the server side summary has a named counterpart to mirror. Resolutions have no read mark of their own and go by their thread's.
Commenting on a thread or resolving one makes it the most recent again, which pushed it to the top of the feed while the reviewer was working in it - reordering the list under their own hands, for something they had just done themselves. Threads now keep the place they had when the list appeared, and coming back to the feed is what reflects the new order. Ordering by the latest activity of others instead would have made the rule depend on whether anyone else had touched a thread: a reply to your own thread would still reorder it, a reply to someone else's would not.
Lets a caller show the opening comment and the tail of a discussion with the replies in between folded into a marker, so a thread can be embedded where there is no room for all of it. A folded thread is a partial view, which is why it neither marks itself read nor offers the reply form. Read state is one timestamp per thread, so marking it would cover comments that were never shown; and replying to a discussion seen only in part invites talking past it. Expanding through the marker settles both.
Each row says what happened and embeds the thread it happened in, with the earlier replies folded away. The latest reply stays visible: the headline announces it, so the row has to show it. Embedding the thread rather than a copy of its text puts both kinds of context a reviewer needs in one place: the row reveals the content the comments are about, and the fold marker reads the discussion without leaving the list. Where the thread sits and what it quotes are left out entirely - both are a click away, and saying them here only crowded the row. The thread carries the click and the highlight, as it does in the structural list, so a row needs no frame of its own. Only the actor's name is emphasized, the way a comment header does it. Shows 30 rows and extends on demand rather than capping the list silently.
Heads the rows of each day, with today and yesterday named rather than dated. Structure the list was missing: rows carried a timestamp each, which said less than a heading does and repeated what the discussion in every thread already dates. The heading also holds the machine readable date the rows gave up, and formats from a timestamp within the day rather than from the day itself - a bare date parses as UTC midnight, which reads as the day before west of it. Groups the shown slice rather than every entry, so a day spanning the show more boundary is not headed twice.
Each row carries the dot that badges and threads already use one level up, and unfolds its thread from the first unread reply rather than showing only the latest one, so the unread part reads in the context of the comment it answers. No separator between seen and unseen rows: read marks are per thread, so a line drawn through a global time order lands somewhere arbitrary as soon as part of what is above it has been read. The date headings give the list its structure and the per row dots carry the exact signal.
Puts the feed on scrolled/comments/activity with a back link, rather than into a third tab of the comments view: the feed is not a scope of the comments list but a different way of looking at all of them. Clicking a row selects its thread the same way the structural list does, so the preview scrolls to it and opens its popover. Since the preview reports selected section threads without a highlighted id, the view keeps the id of the clicked row to survive the round trip.
Adds a control strip above the tabs holding a link to the activity route, carrying the dot the comments main menu item already uses so that unseen comments are visible from within the comments view too. The strip is laid out with room on the left: the resolution filter the preview's floating toolbar offers is the intended next occupant. It stays outside the sticky tab bar until it actually holds filters.
Selecting a subject in the preview keeps the sidebar where it is once it is already showing comments, so that stepping through them does not tear the reviewer off the tab they chose. The activity feed sits under the same route but lists every subject at once, so a subject picked there had nowhere to appear, and the selection went unanswered. Threads clicked in the sidebar are the exception: they travel to the preview as a message and come back as a selection, which now says so, so that the view that asked is left alone.
A collapsed thread showed nothing of its resolution: the row carrying it was hidden along with the replies, so a list of resolved threads read as a list of ordinary ones. The state now stays whatever the thread shows of itself, undoing it included. Only the offer to resolve keeps waiting for the thread to be expanded.
Stepping to the next comment and jumping to a particular one differ only in how they pick the target, not in what selecting it involves.
The feed the editor sidebar shows on its own route was out of reach while reviewing in the preview. A toolbar button now opens it as a panel above the toolbar, and clicking an entry reveals the thread in place - jumping to it the way the arrows do, excursions and all, and switching the filter where a resolved thread would otherwise stay behind the toolbar's own. The panel goes into the portal layer that sits above the navigation widgets, since it grows tall enough to reach them. That also keeps it out of the toolbar, whose collapse would otherwise snapshot it, and clear of the floating ui layer the popovers use, which is below the toolbar and would swallow it.
The puck counted unread comments while the activity button beside it counts topics carrying something new, so the same toolbar could show two markers that disagree - a thread someone resolved without commenting lit one and not the other.
A row in the feed shows its thread already, so opening the popover on top of it said everything twice and covered the content the click had just revealed. The selection now carries whether it only reveals: the badge goes active and the subject scrolls into view, while the thread list stays where the reviewer was reading it. Clicking the badge or the highlight of a revealed subject opens the popover after all, on the thread it was revealed for, rather than clearing the selection or doing nothing.
A thread with no replies yet shows its reply form even where the list keeps every thread summarized - there is nothing folded away to hide it. Sending the reply gave the thread its first one, and with it the fold that swallowed what had just been written; the draft held the thread open only until the session dropped it. The list now takes the reply as the reviewer picking that thread, the same way clicking its count does.
Being on screen used to be enough, and anything the thread kept out of sight - collapsed or folded replies - switched it off. Both halves were wrong at the ends of this stack. The feed folds away exactly the replies the reviewer has already seen, so a row it could never clear was hiding nothing; and a whole entry list scrolled past clears markers the reviewer navigates by, spending them on threads nobody looked at. A thread is read once everything unread in it has been on screen, and, in the lists that survey many threads at once, once the reviewer has picked it out. That the entry list wants its threads expanded before they count, and the feed does not, falls out of the first half: the fold hides nothing unread, a collapsed thread hides its replies.
The dot in the corner locates one unread thread among several, which is what it was for, but it takes finding. A thread whose topic has not been read is new all through, so the whole card can say so: its border takes a wash of the dot's colour, and the folded seam with it. A thread that only gained replies keeps its border - what is new there is the count and the divider, not the conversation. The warning colour gains the lighter and lightest variants every other colour in the palette already has. The outline outranks the hover and highlight borders. What is left to read is worth more than where the reviewer is, which the highlight halo goes on showing either way.
The badges in the text column build their own, since they resolve a thread from a Slate range rather than from a subject, and so they never passed the badge an unread count. The dot stayed off, and worse, the escape that keeps a badge with unseen comments from shrinking to a dot never fired: unread text comments were the least visible ones in the editor. Badge takes the count now instead of a flag and the sentence naming it, which both callers were otherwise left to phrase alike.
Read state froze for as long as a list was on screen, which kept a thread marked new long after it had been read: the reviewer worked through a list and nothing came off it until the list went away. The freeze is there so markers do not vanish mid-read, and that only asks for them to hold still while a thread is being read - not for the whole visit. The list now freezes anew whenever the reviewer expands another thread or collapses the one they were reading, so what they just read loses its marker as they move on. The popover's snapshot goes with it. It wrapped the badge and the list together, and since a nested snapshot defers to the one outside it, the list could never refresh. The badge has no call to hold read state of its own either: it is the one thing in there the reviewer is not reading, and it says whether a subject still has something left - which is worth more the moment it stops saying so. The badges in the editor have always shown live state.
Somebody marking a thread resolved is worth as much as a comment saying so, and it was the one thing the reviewer could miss. The feed counted it; everything else counted comments. A thread whose comments had all been read but which somebody had just resolved lit the feed's indicator with nothing to show for it in the row, and could never be marked read again - the read signal only arms itself while comments are unread, so the indicator stayed lit for good. One rule now: a thread's activity is what was said in it plus its resolution, and whatever of that the reviewer has not seen is unread. The thread's dot, the badges, the read signal and the main menu's indicator all ask the same question, and unreadComments.js has become unreadActivity.js to say so. The count beside a badge no longer names what it counts - "1 unread" rather than "1 unread comment" - since one of them may be a resolution.
The admin's indicator went by comments alone and only ever looked at unresolved threads, so the one thing worth not missing was invisible there: a thread somebody had just resolved dropped out of the query along with its resolution. It counts now, beside the unread topics and replies, and the indicator shows even where no topic is left open - the last one being resolved is exactly the case. The rule stays in step with the review interface: the resolution goes by the thread's read mark, having none of its own, and the user's own resolutions never count.
The line saying a thread was marked as resolved looked the same whether the reviewer had seen that happen or not, which left the one event with no comment to carry it silent inside the thread. It takes the colour the dots and counts use when the resolution is what is unread - icon, wording and resolver alike, since the whole line is the news. Thread activity says which of its events is the resolution rather than leaving it to be the one without a comment id.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
REDMINE-21261