flake8
failed.
JSHint
passed.
Improve the logic for collapsing any entry with a review.
Review Request #9281 — Created Oct. 18, 2017 and submitted
The logic for computing the collapse state for reviews (and anything
containing reviews by way of status updates) has been updated to better
show off the entries that users most likely want to see.The new logic will expand review entries (or anything containing
reviews) if:
- The review is being seen by the user for the first time
- There's open issues (including those in verifying states)
- There's a new published reply
- There's a pending draft replying to a body field or a comment
Much of this is the same as before, but there are some notable changes:
Previously, only draft replies to comments were considered when
determining the collapsing state. Draft replies to body top/bottom
fields were not included.Review entries are no longer automatically expanded if newer than the
latest change description, or automatically collapsed if older than
it. The change description is no longer consider for review entries
at all, providing more focus on the updates to the entries
themselves.For the Initial Status Updates entry and Change entries containing
status updates, only draft replies to comments were previously
considered. Now, the collapsing logic for reviews apply to these
as well when there are status updates with reviews.Thanks to Connor Yoshimoto for prototyping the new collapsing logic for
the page.
Unit tests pass.
Tested with a very comprehensive review request I have that has entries
of all kinds in all states needed for testing. Checked each entry and
saw that the collapsed states all made sense with regards to the new logic.
| Description | From | Last Updated |
|---|---|---|
|
F841 local variable 'reply' is assigned to but never used |
 reviewbot
|
|
|
F841 local variable 'reply' is assigned to but never used |
reviewbot
|
|
|
F841 local variable 'reply' is assigned to but never used |
reviewbot
|
|
|
F841 local variable 'reply' is assigned to but never used |
reviewbot
|
|
|
F841 local variable 'reply' is assigned to but never used |
reviewbot
|
|
|
F841 local variable 'reply' is assigned to but never used |
reviewbot
|
|
|
Now that we have better knowledge about the reviews within, we can do better here. Instead of always expanding if … |
 david
|
|
|
This could be simplified to just say "there aren't any status updates with reviews that should be expanded". That covers … |
david
|
|
|
F841 local variable 'reply' is assigned to but never used |
reviewbot
|
- Change Summary:
-
Addressed Review Bot complaints.
- Commit:
-
52cd3343eafa2666b875f776f6cc808eabaf313f7ec0ef9134167b704a344a1c7838daafca599022
-
-
Now that we have better knowledge about the reviews within, we can do better here.
Instead of always expanding if there aren't any change descriptions, I think it would be better to check the last read timestamp against the timestamps of the initial status updates. That way it behaves the same as the other entries--if the user hasn't seen it, or if any of the reviews within have something new or important, it's expanded.
-
This could be simplified to just say "there aren't any status updates with reviews that should be expanded". That covers the case where there aren't any status update at all.