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

message view: Use inline date separators. #10820

Merged
merged 1 commit into from Feb 11, 2019

Conversation

showell
Copy link
Contributor

@showell showell commented Nov 12, 2018

There may be a simpler way to check czo here, but the
main goal here is to just deploy-test the one-liner to
get some immediate user feedback.

Testing Plan:

GIFs or Screenshots:

@showell
Copy link
Contributor Author

showell commented Nov 13, 2018

The first two commits here can go straight to master if you want, but the third one might be worth a test deploy.

@timabbott
Copy link
Sponsor Member

I merged the test fix; can do a test-deploy late today or maybe tomorrow.

@timabbott
Copy link
Sponsor Member

OK, test-deploying now.

@timabbott
Copy link
Sponsor Member

See https://chat.zulip.org/#narrow/stream/101-design/subject/date.20separators/near/661609. I also found a weird bug where EDITED appeared somewhere totally weird:

image

@timabbott
Copy link
Sponsor Member

So I think we have support for the concept but a handful of bugs reported by multiple people:

  • We should still be showing sender headers for messages on a new date.
  • The thing I posted above with EDITED positioning.
  • Floating recipient bar should update when you scroll past a date separator.

@timabbott
Copy link
Sponsor Member

(Merged the second refactoring commit, now that I have a bit more time)

@timabbott
Copy link
Sponsor Member

(See also #10839)

@showell
Copy link
Contributor Author

showell commented Nov 27, 2018

I rebased, but I haven't addressed any of the feedback yet.

@showell showell force-pushed the showell-november-date-links branch 3 times, most recently from 76b270e to 67b6d92 Compare December 26, 2018 15:13
@showell showell changed the title DEPLOY TEST: Avoid date separators on czo. message view: Use inline date separators. Dec 26, 2018
@showell
Copy link
Contributor Author

showell commented Dec 27, 2018

The first three commits here are refactoring commits, and they all probably make sense regardless of how the experiment turns out.

I wasn't able to reproduce the EDITED things, but I think I may have fixed it as part of re-working my changes, plus there was some plausibly related recent activity on master that I picked up in rebases. Tim, you should definitely test this out, since you reported the bug and may have some particular behavior that triggers it.

The last two commits change the actual UI here, and they can arguably be squashed together. The first commit puts the date separators inside groups, not between them. Then the second commit fixes the problem that @jackrzhang reported in #10839, and we now update the FRB as we scroll.

I made a couple decisions here that I think are pretty defensible, but we'll see if it elicits feedback:

  • I now always show dates in recipient bars. I think this makes sense in general, but it particularly makes sense in a world where use the slimmer date separators within groups that span multiple days.
  • I don't put date separators between groups. I think the recipient bars, with the dates, make them a bit redundant. For the first group, you can visually scan up to either a date separator inside, or the date will show in the recipient bar (and if it's floating, it will be there, too). And then for the second group, there will be a date in the recipient bar.

@showell
Copy link
Contributor Author

showell commented Dec 27, 2018

image

@timabbott
Copy link
Sponsor Member

I merged the 3 prep commits and push back to this PR.

@timabbott
Copy link
Sponsor Member

I now always show dates in recipient bars. I think this makes sense in general, but it particularly makes sense in a world where use the slimmer date separators within groups that span multiple days.

I think this decision we'll likely want to undo, in that we did the suppression of dates there when they are irrelevant as part of an effort to declutter the UI, and that change was popular at the time.

Changing this just requires bringing back the show_date value and setting it correctly, right?

@timabbott
Copy link
Sponsor Member

@rishig can you play around with this and let me know if you can find any bugs and what you think about the UX? It seems worth doing a round of that before test-deploying on chat.zulip.org.

@rishig
Copy link
Member

rishig commented Jan 4, 2019

cool!

  • I occasionally get browser errors of the form source_recip_bar is not defined while trying to navigate with up/down arrows over the recipient bars (couldn't figure out a repro, but has happened twice while testing).

  • Getting this when reaching top of narrow (pressing up arrow doesn't fix it, but reload does):
    image
    I think a repro is: go to a topic narrow that's more than 1 screenlength long. Use up arrow till you reach the top.

There are probably a number of styling changes we'll want to do, but I think the most noticable one is that the bar feels uncentered; i.e. too much bottom-margin relative to top-margin.

Only if it's not too hard, we could also do the following before testing on czo:

  • Have only 1 date (the new one)
  • Move it to the right side, in line with the other dates and timestamps
  • The date can be a bit bolder, and use sentence-case, similar to the date in the recipient bar

@showell
Copy link
Contributor Author

showell commented Jan 4, 2019

And then styling is in static/styles/zulip.scss (although maybe should be moved, but doesn't have to happen as part of this effort):

2646 .date_row .date-line {
2647     display: inline-block;
2648     vertical-align: middle;
2649     width: 33%;
2650     border-top: 1px solid hsl(0, 0%, 88%);
2651     border-bottom: 1px solid hsl(0, 0%, 100%);
2652     margin: 0px 5px 0px 5px;
2653 }
2654
2655 .sub-unsub-message span,
2656 .date_row span {
2657     display: block;
2658     background: inherit;
2659     padding: 4px;
2660     overflow: hidden;
2661     text-transform: uppercase;
2662     font-size: 0.8em;
2663     opacity: 0.5;
2664 }

@rishig
Copy link
Member

rishig commented Jan 4, 2019

Played around with this; looks good to me! I think ready for pushing to master or czo, and then we can fix-forward on the styling.

@timabbott
Copy link
Sponsor Member

OK, just test-deployed on chat.zulip.org.

@timabbott
Copy link
Sponsor Member

Styling-wise, I think we want to remove the margin-bottom: 10px on .date-row on line 2637 of zulip.scss.

@timabbott
Copy link
Sponsor Member

I squashed that change in, and added a fixes tag for #10171, and pushed back here. Test-deploying to czo again.

@timabbott
Copy link
Sponsor Member

I'm not really sure what you mean by "sentence casing" for the dates--don't we already do that?

I think this part is about the small caps case used in the date-line thingys.

@timabbott
Copy link
Sponsor Member

After playing with this a bit, I like the overall experience, so we should definitely merge something in this direction. I still really want #10820 (comment) to be changed back. Interleaved views like this (which is the only thing that decision affects) now have the date part look really spammy:

image

@rishig
Copy link
Member

rishig commented Jan 5, 2019

Agreed that removing the extra dates makes sense, even after this change.

@timabbott
Copy link
Sponsor Member

OK. @showell can your refactor this to preserve the existing behavior for the recipient bar date fields (I understand this may require some logic changes)? I think after that, we'll be ready to merge.

@showell showell force-pushed the showell-november-date-links branch from 37f609a to 541aa94 Compare January 8, 2019 04:00
@showell
Copy link
Contributor Author

showell commented Jan 8, 2019

@timabbott and @rishig This is ready for another round of testing. I de-clutter the dates.

Tim, I have commits split out for helping the review, but they should technically be squashed back together.

De-cluttering the dates motivated a significant rewrite here. Part of the complications with de-cluttering dates under the new paradigm here is that the FRB for any section can change on the fly. You could theoretically just update the "next" recipient bar every time, as well as the old FRB's neighbor, but this gets edge-case-y really fast. Instead, I just recompute the info for all recipient bars that are visible during every update call. (Although I didn't implement it yet, I think we could also speed up compose-fade using the relevant_recipient_bars function.)

Technically the floating_recipient_bar module has grown in scope here to be more like recipient_bar, but the diff's already complicated enough.

@showell
Copy link
Contributor Author

showell commented Jan 8, 2019

P.S. Tim, I possibly regressed the small CSS tweak you made, not sure. It should be easy to figure out with the current break out of commits, as all my work here was additive (but needs to be squashed).

@timabbott
Copy link
Sponsor Member

I think you didn't -- looks OK. This has been test-deployed on chat.zulip.org for today without report of issues, so I'll try to integrate it first thing tomorrow.

@showell
Copy link
Contributor Author

showell commented Jan 12, 2019

Cleanup:

  • remove tests related to dead field show_date_separator
  • remove else in 'if where === top' block
  • squash into one commit
  • re-apply Tim's CSS change (see above)
  • look at mentions

@timabbott timabbott self-assigned this Jan 12, 2019
@timabbott
Copy link
Sponsor Member

@rishig just to update you on this PR,

Tim Abbott: Well, I think the actual design we're talking about here is this:

  1. In the current master design, each date value (Today, Yesterday, etc.) appears at most once in the right-hand column, in either a floating recipient bar or a normal recipient bar. This is achieved by breaking recipient bars for each date change, in the static rendering.
  2. In the current czo / this branch design, each date value (Today, Yesterday, etc.) appears at most once in the right-hand column, in either a floating recipient bar or a normal recipient bar. This cannot be achieved statically, and so we dynamically make it happen via rerendering when processing floating recipient bar updates. This produces a glitch when scrolling up in the "all messages" or a stream view, where a bunch of Today strings flash briefly in the date area for the 50ms until the floating recipient bar logic runs, and then they disappear. We could potentially make this glitch much less visible by adjusting the default state of these elements in the rendering code to be likely the value they're going to end up at.
  3. The proposed alternative design that I think we eventually want to get to requires restyling the date-change bars so that the place they show today's date is in the far-right column where we put timestamps. Once that's done, it would again be possible to statically render the message list's decisions of which recipient bars show a date, with each date appearing at most once, but we'd need somewhat more complicated logic for both the floating recipient bar and this static rendering to take into account date rows in addition to recipient rows.
Option 3 would involve changing this:
-------------------------- < June 14 ------ > June 15 ------------------------------

to instead something like this:

-------------------------------------------------------------------------------------------  June 14

I think barring us doing that styling change, this model is about the best we can do (though as I mentioned, I think we could choose the hide/show state for dates in the initial rendering better, such that rather than every message starting with the date appearing and that disappearing 50ms after you scroll the message into view, they instead start out with something more similar to the old static decisions for whether to display dates, and auto-correct that -- that would make the glitch like 30x less visible)

@rishig
Copy link
Member

rishig commented Jan 19, 2019

Makes sense to me. I think the dates not appearing at all for 50ms is also probably fine.
(And yeah, looking forward to the styling change of option 3).

This adds date dividers within a single message group when the only
reason we had previously been splitting apart two message groups is a
change of date.  The overall effect is a cleaner message list user
experience.

The downside of this change would be that the recipient bars no longer
will always show a new date for date changes; to fix that, we rewrite
how the floating recipient bars both set the date field on the
floating recipient bar itself, as well as ensure that non-floating
recipient bars don't show duplicate dates.

In a future design update where we modify how message recipient bars
look, we may very well be able to simplify this logic by removing some
of the dynamic nature of the recipient bar calculations.  But this is
a good implementation of what remains.

Tweaked significantly by tabbott from Steve Howell's original, both to
extract these changes from a larger PR as well as to modify the
first_visible_message logic to handle some tricky corner cases.

Fixes zulip#10171.
@timabbott timabbott merged commit 51c6c82 into zulip:master Feb 11, 2019
@timabbott
Copy link
Sponsor Member

OK, I finished cleaning this up and merged it. @showell FYI, I fixed a few things:

  • Left in place the between-recipient rows date dividers, since for the moment those are net helpful.
  • Fixed candidate_recipient_bars to handle there being non-recipient-row things between them. This required debugging the annoying fact that .prev(selector) returns nothing if the selector doesn't apply (not the next child that matches!).
  • Adjusted the date row selection logic in first_visible to handle two tricky corner cases where we were either displaying no date at all in the floating recipient bar incorrectly or displaying a confusing date (when the floating recipient bar partially obscures a message feed recipient bar).

Huge thanks for building this @showell! I think it makes zulip's UI look a lot cleaner.

andersk added a commit to andersk/zulip that referenced this pull request Feb 13, 2019
Fixes border-related glitches introduced by commit
51c6c82 (zulip#10820).  Alternative
to zulip#11534.

Signed-off-by: Anders Kaseorg <andersk@mit.edu>
andersk added a commit to andersk/zulip that referenced this pull request Feb 13, 2019
Fixes border-related glitches introduced by commit
51c6c82 (zulip#10820).  Alternative
to zulip#11534.

Signed-off-by: Anders Kaseorg <andersk@mit.edu>
timabbott pushed a commit to timabbott/zulip that referenced this pull request Feb 14, 2019
Fixes border-related glitches introduced by commit
51c6c82 (zulip#10820).  Alternative
to zulip#11534.

Signed-off-by: Anders Kaseorg <andersk@mit.edu>
Raghavareddy21 pushed a commit to Raghavareddy21/zulip that referenced this pull request Feb 18, 2019
Fixes border-related glitches introduced by commit
51c6c82 (zulip#10820).  Alternative
to zulip#11534.

Signed-off-by: Anders Kaseorg <andersk@mit.edu>
ericgumba pushed a commit to ericgumba/zulip that referenced this pull request Mar 2, 2019
Fixes border-related glitches introduced by commit
51c6c82 (zulip#10820).  Alternative
to zulip#11534.

Signed-off-by: Anders Kaseorg <andersk@mit.edu>
Nikhil-Vats pushed a commit to Nikhil-Vats/zulip that referenced this pull request Mar 17, 2019
Fixes border-related glitches introduced by commit
51c6c82 (zulip#10820).  Alternative
to zulip#11534.

Signed-off-by: Anders Kaseorg <andersk@mit.edu>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants