@gnprice Thanks for the review, have addressed the concerns on my machine! Sorry about the screenshot, I wasn't ready for a review right now 😅, regardless I'll add screenshots/suitable recordings to this shortly. Been busy lately, didn't have enough time to dedicate to the project.

Another thing, I had originally marked this as a WIP because the CSS and some JS might change because of the complexity of implementing this in topics/1:1 pms, which is why I was holding back on asking for a review.

Got it, no problem 🙂 When you'd like another review, just leave a comment here!

I think this is ready for review! I have detailed what has been done to solve each checkbox in the relevant commit, which hopefully is understandable 🙂 .

Recordings are as follows :-

Also, thanks for those recordings! Very helpful 🙂

In the 1:1 PM case, the bar looks oddly blank with just the date at the end -- it looks like there's some kind of bug. (Reminds me of how some paper exams, forms, etc. will say "This Page Intentionally Left Blank" rather than actually leave a page blank, to avoid people worrying that there was some error in printing.) Not immediately sure what'd be the best way to address that, though.

@borisyankov @gnprice thanks for the reviews! I have addressed all the points covered and made the appropriate changes.

The failed check is because of the changes introduced in the first commit with using caseNarrow. Unfortunately I do not have enough time today to look into fixing it, but I think it should be done by the day after tomorrow.

I will be posting updated screenshots after the design is finalised and implemented, so that work done in capturing and posting these is not repeated.

The design finalisation I am talking about is the problem concerning how dates should be shown in the PM narrow (as said in the last comment). I've given it some thought and my opinion is to keep the entirety of the header (the background) and the header text is aligned to the middle (optionally giving it a defined width with rounded corners, although I don't think this would go well with the rest of the app). The reasoning behind this is that we cannot remove the name from the appbar because that would involve removing the last active indicator, which is not a desired trait, and we cannot stick the date to the right due to the reasons explained in the comment above.

I'm not too sure about the design though, if there are better ideas I would like to hear them.

I was definitely not a fan of adding the headers in the narrows that previously didn't have one.
Especially disliked the one in topic narrows, that moves the topic name there too.

The first issue I have is that they are just very confusing, you expect this to be a separator that just got stuck at top temporarily but in these narrows it is a permanent thing that is also empty (besides the date on the right).

A smaller issue, these take vertical space that previously was free for display of messages.

Another thing that we have to improve:

• now we have duplication of 'time rows' and these dates
• we are still not showing the actual time of the message (the sticky headers are not a great place for this, just noting it as an outstanding issue)

I don't see a reason not to merge the changes for the narrows already with sticky headers, as these look great.

We can iterate on the other narrows more though. Maybe a sticky date that only takes the right side of the screen (basically a header that is something like { width: 100, right: 0 } in css.

Thanks @ishammahajan for these revisions!

• The first commit, making that headerStyle refactor, looks great. I'm about to merge it, with two adjustments:

  • The two test failures it caused were because a set of tests are passing half-formed, ill-typed data, causing this function to get undefined for narrow. (Many of our tests don't pass the type-checker, and so they don't have the /* @flow */ marking that causes the type-checker to report errors in them.) I fixed up the tests to pass a valid Narrow value.
  • I added a small second commit on top that organizes the different narrow types to I think make the patterns a bit easier to see, and also adds a type annotation to help Flow more precisely understand the code.

• On the design:

  I will be posting updated screenshots after the design is finalised and implemented, so that work done in capturing and posting these is not repeated.

  The approach I think works best is: any time you make visible changes to the design which you'd like to discuss, make new screenshots and post them. They really help a lot in getting everyone (even just you and one reviewer) on the same page about what designs we're talking about.

  To keep the amount of work down, it'd be perfectly fine to post just one or two still screenshots if that illustrates what's changed. The original set of three anigif captures is great for seeing the design in full context, but the reader can combine those with a still to get a picture of the new design.

• Haven't yet read in detail the second and later commits -- I'll do that next.

• The first commit, making that headerStyle refactor, looks great. I'm about to merge it, with two adjustments:

And merged! This is a6b4b5e and b1b873f .

On the next commit, modifying the header only for stream messages:

• Careful with changing where we do and don't HTML-encode things:

-  $!${topicHtml}
+  <div class="topic-text">${topicHtml}</div>

It's true that $!$, which uses data without HTML-encoding it, is dangerous because skipping HTML encoding means HTML injection... but if the data is already HTML, then HTML-encoding it a second time is also a bug, though a less severe one.

With this change, if the topic is actually "2 + 2 < 5", it gets doubly encoded to become "2 + 2 &lt; 5", which means it displays as "2 + 2 < 5".

I just wrote and pushed to master a quick test that demonstrates this -- see #3467. When you rebase on the new master, you'll find that this change breaks that test.

Same thing in the other case here, even though I didn't add a test for that today.

• This is still a bit confusing to me:

-<div class="header-wrapper stream-header" data-msg-id="${item.id}">
+<div class="header-wrapper header stream-header topic-header"
+    data-msg-id="${item.id}"
+    data-narrow="${topicNarrowStr}">
   <div class="header stream-text"
        style="color: ${textColor};
               background: ${backgroundColor}"
        data-narrow="${streamNarrowStr}">
     # ${item.display_recipient}
   </div>
-  <div class="header topic-text" data-narrow="${topicNarrowStr}">
-    $!${topicHtml}
-  </div>
+  <div class="topic-text">${topicHtml}</div>
+  <div class="topic-date">${humanDate(new Date(item.timestamp * 1000))}</div>
 </div>

One thing: what is topic-header intended to mean?

Also the nesting of header is confusing. I think the effect will be that if you tap on the stream name, it'll still narrow to the stream, but if you tap anywhere else in the header (the topic, the blank space, the date, or possibly some stray padding somewhere even next to the stream name) it'll narrow to the topic.

Is it intentional that tapping on the date will narrow to the topic? If so, that should definitely be explicit in the commit message. There may also be a cleaner way to do that that doesn't involve this nesting (and the worry that maybe some edge around the stream name will also narrow to the topic.)

• In the CSS, there are two .topic-header blocks, and they contradict each other on background.

@borisyankov thanks for the insight on design!

not a fan of adding the headers in the narrows that previously didn't have one

I have continued the discussion in zulip about this, let's discuss the points further over there. The gist of my opinion about this is that, while it does sacrifice a tiny bit of the vertical space, it brings uniformity to the design, since now headers appear everywhere and actually have a function which makes them non-static. It also gives the added benefit of being in sync with the webapp's design.

The possible designs are the two following and everything in between.

you expect this to be a separator that just got stuck at top temporarily but in these narrows it is a permanent thing that is also empty

I do agree with this, we don't yet have a solution for showing dates in private headers, and while I have posted some mockups of a possible design in chat, I do think there may be better solutions out there.

• now we have duplication of 'time rows' and these dates

I do think both are necessary, since if there is a case where the timerow is in the middle of the screen, the date on the header will not reflect that of the timerow which is on the screen, it will rather reflect the date of the timerow above it.

It would also be applicable in another case where the topic of the conversation does not change as the clock strikes midnight. In this case since the topic doesn't change, another header is not created, but a timerow is still required because we do still need to display the date.

The webapp handles this similarly as well.

• we are still not showing the actual time of the message (the sticky headers are not a great place for this, just noting it as an outstanding issue)

I'll be working on that next, as soon as design on this is finalised and this PR is merged :)

@gnprice thanks for pushing the fix for the first commit!

With this change, if the topic is actually "2 + 2 < 5", it gets doubly encoded to become "2 + 2 < 5", which means it displays as "2 + 2 < 5".

It gets doubly encoded to become "2 + 2 &lt; 5", right? Single encoding would bring it to "2 + 2 < 5".

I think the effect will be that if you tap on the stream name ...

That's exactly right! I wanted to make the structure such that the area a user taps to get to the stream (the stream name # stream-name) would get superimposed on the header for the topic itself. This way the headers are more modular and if there was another change required to header (say) it would be easy to think of/make. If the headers are not nested, this modularity is non-existent. The topic-header is a 'type' of header, just like stream-header and private-header.

• In the CSS, there are two .topic-header blocks, and they contradict each other on background

They do contradict each other on colours, but that's because one of the colours is for dark theme and one is for light (I haven't changed anything with respect to the colours. As the commit says, I have simply renamed what was topic-text to topic-header, topic-text also had apparently contradictory blocks.)

--
I have rebased onto master and made changes accordingly. Tested again, seems to work well. Screen recordings are almost exactly the same as the previous with changes to transparency of the date and a padding introduced between topic-text and the date so as to not make the header congested.

@gnprice thanks for pushing the fix for the first commit!

With this change, if the topic is actually "2 + 2 < 5", it gets doubly encoded to become "2 + 2 < 5", which means it displays as "2 + 2 < 5".

It gets doubly encoded to become "2 + 2 < 5", right? Single encoding would bring it to "2 + 2 < 5".

Oof, this is confusing because GitHub decodes these entities in comments... so in the rendering of my comment things come out differently than how I'd typed them. I didn't notice that until just now -- let me try again, using the "preview" feature to make sure it comes out right:

With this change, if the topic is actually "2 + 2 < 5" (with a less-than), it gets doubly encoded to become 2 + 2 < 5 (the less-than replaced with a sequence of 8 characters including two semicolons), which means it displays as "2 + 2 &lt; 5" (the less-than replaced with 4 characters including one semicolon.)

(On top of everything else, in fact GitHub apparently has an HTML-encoding bug of its own 🤣 😢 where the "Quote reply" feature copies the displayed text version, without encoding things back as HTML entities... so they get decoded a second time, and your quote from my comment comes out differently from my actual comment, and the same with my quote from your comment.)

Does that make better sense now? Thanks for pointing out the confusion 🙂

• In the CSS, there are two .topic-header blocks, and they contradict each other on background

They do contradict each other on colours, but that's because one of the colours is for dark theme and one is for light (I haven't changed anything with respect to the colours. As the commit says, I have simply renamed what was topic-text to topic-header, topic-text also had apparently contradictory blocks.)

Ah I see, makes sense, thanks. I saw the two in the diff (with just the usual 3 lines of context), and mistakenly assumed they were both in the one giant block.

I think the effect will be that if you tap on the stream name ...

That's exactly right! I wanted to make the structure such that the area a user taps to get to the stream (the stream name # stream-name) would get superimposed on the header for the topic itself. This way the headers are more modular and if there was another change required to header (say) it would be easy to think of/make. If the headers are not nested, this modularity is non-existent. The topic-header is a 'type' of header, just like stream-header and private-header.

Hmm, I see! And thanks for the added explanation in the commit message; that's quite helpful.

I like the idea of this being a modular piece that's dropped in. A couple of things that make it not quite there even with this change (and these would be a good target for followup commits):

• The stream-header class. I'm not certain this is actually doing anything for us at all -- the display: flex is definitely redundant, and the two other attributes may be too.
• The order of the data-narrow and data-msg-id attributes, and the whitespace before the closing >. These have no semantic effect at all... but they do get in the way of seeing the parallelism between these cases, as a human reader 😉

I just pushed the next commit, adding dates in the existing recipient headers for stream messages. Thanks again @ishammahajan !

I also added a small commit on top that fixes the bug introduced. In general we avoid having any given commit break things, even if a later commit in the PR fixes it -- and this is an illustration of one reason why, as we're not ready to merge that next commit. 🙂

Another reason we avoid having any given commit break things is that it's generally a lot easier to read and understand the commit when you can assume the author and reviewer(s) believed it all made sense together and didn't break anything. (There can always still be bugs they missed, of course, but at least you have a much better shot at following what those humans were thinking.) So the clear identification of the bug in the commit message mitigates that, and was crucial in it being OK to merge the commit with the known bug. But I still didn't want to merge it without also promptly fixing it. 😉

(For design discussion for the next steps, I'll move to that chat thread you created.)

Also, FTR: the commits I just merged were ada59a7 and 123fd7a.

Oh, one next step that I think we can do before settling further design questions: PM headers, in the cases where they already appear (namely "special narrows" like all-messages, starred, and search.) In those narrows, it looks a bit incongruous to have dates on headers for stream messages but not PMs.

This is basically one half of the changes that appear in your next commit:

msglist header: Modify pm/group headers to show date.

but that commit also starts showing those headers in PM narrows, which I'd prefer to hold off on for a bit more design discussion. Would you split that change out as a separate first commit?

I think that split makes things a bit clearer even if we were about to merge them both, though it wouldn't be absolutely essential in that case. (One thing I would insist on even then, though: the commit message should definitely be clear that it's making both those changes. In the current one it's easy to miss that and read it as only being about adding dates to existing headers.)

Does that make better sense now? Thanks for pointing out the confusion

It does 😂 ! My reply to your comment was based on the me trying to understand the test anyway.

• The order of the data-narrow and data-msg-id attributes

I'll fix that promptly, thanks!

• The stream-header class. I'm not certain this is actually doing anything for us at all -- the display: flex is definitely redundant, and the two other attributes may be too.

Just tested it, it appears as if the class is completely redundant, since removing it from messageHeaderAsHtml.js doesn't change anything.

Would you split that change out as a separate first commit?

I'll do that, unfortunately not today though, since there's not enough time.

Closing due to #3496.