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

A proper unread marker #332

Merged
merged 2 commits into from
Jul 6, 2016
Merged

A proper unread marker #332

merged 2 commits into from
Jul 6, 2016

Conversation

xPaw
Copy link
Member

@xPaw xPaw commented May 14, 2016

This PR has two issues:

  • Doesn't work correctly if unread message is in the unloaded buffer
  • Need to look into using document visibility API.

This PR however has proper server state tracking, so the marker isn't lost between page loads.

@xPaw xPaw added the Type: Feature Tickets that describe a desired feature or PRs that add them to the project. label May 14, 2016
@xPaw xPaw mentioned this pull request May 14, 2016
@xPaw xPaw added the Meta: Do Not Merge This PR should not be merged. label May 14, 2016
@maxpoulin64
Copy link
Member

Looks good to me. My only comment is that I think the unread bar could be a bit more subtle and thicker. I liked the one from #169 better for that, because it doesn't constantly grab your attention.

Also probably should have one per theme. I'm going to guess yours is based on morning considering the blue color :)

@xPaw
Copy link
Member Author

xPaw commented May 14, 2016

I didn't really mess with the styling just yet, I wanted to get a working prototype out. I still don't know how to deal with unread marker not showing up for hidden messages (joins etc).

@richrd
Copy link
Member

richrd commented May 14, 2016

Regarding hidden messages: Using :after for the implementation is something that I like and I think is rather elegant. Unfortunately it's probably not directly possible with hidden elements. (http://stackoverflow.com/questions/14049423/in-css-use-displaynone-on-the-element-but-keep-its-after)

If a pseudo element is required I would suggest hiding child elements with something like this: #chat .messages .msg * {display: none} That way the pseudo element will be shown. I guess one of the other options would be adding a 'real' element before the first unread message (or action etc).

Just my 2 cents. And thanks for all the work you guys have put in to this project, love it! 👍

@xPaw
Copy link
Member Author

xPaw commented May 14, 2016

Your * approach won't work due to the table layout chat uses.

@richrd
Copy link
Member

richrd commented May 15, 2016

I tried using #chat .msg.join * {display: none} in Chrome via the inspector, and it worked fine for me. The unread marker was shown but the join line wasn't.

@xPaw
Copy link
Member Author

xPaw commented May 15, 2016

Hmm. I remember fixing an issue that was related to hiding joins and the layout messing up, but I can't remember what it was exactly.

EDIT: I think this is it: 8bbb0ab #227

@xPaw xPaw force-pushed the xpaw/unread-marker branch 2 times, most recently from f221212 to 414d803 Compare May 28, 2016 11:14
@xPaw
Copy link
Member Author

xPaw commented May 28, 2016

I've updated the PR to create a separate marker line in DOM, the solution ends up being cleaner over all using this method.

if (data.firstUnread > 0) {
var first = $("#msg-" + data.firstUnread);

// TODO: If the message is far off in the history, we still need to append the marker into DOM
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this TODO still relevant? Seems you do append the marker (well, prepend) to the DOM? I think you just need to move it to the right place in the on("more", handler right?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes it's still relevant. To move it on more handler, firstUnread needs to be persisted then, the unread marker has to remain in DOM and hidden. But as we consider channel read as soon as we open it (not when we load the unread message), this becomes slightly more trickier.

@xPaw xPaw removed the Meta: Do Not Merge This PR should not be merged. label Jun 6, 2016
@maxpoulin64
Copy link
Member

Okay, been using it for the day. Just dumping some notes for now, I will truely investigate later probably tomorrow.

  • I think the marker gets trimmed with the other messages when the backlog gets too big. When I either restart the server (and ZNC plays the buffer back) or I've been in a channel for too long, the marker just stops working. I'll have to investigate further to confirm, just observations so far.
  • The marker is never visible on mobile because the spans are no longer table cells. I removed the spans and just gave the div a height in custom CSS as a workaround and it worked.

@xPaw
Copy link
Member Author

xPaw commented Jun 7, 2016

@maxpoulin64

  1. I think I fixed this.
  2. Fixed.

@williamboman
Copy link
Member

Been having this issue (might be running on a very old version of this PR though)

@xPaw
Copy link
Member Author

xPaw commented Jun 7, 2016

@williamboman Are you hiding joins or anything like that? I assume that would break the last-child selector

@williamboman
Copy link
Member

Yeah I hide join/parts etc.
On 7 Jun 2016 20:47, "Pavel Djundik" notifications@github.com wrote:

@williamboman https://github.com/williamboman Are you hiding joins or
anything like that? I assume that would break the last-child selector


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#332 (comment), or mute
the thread
https://github.com/notifications/unsubscribe/AGZQCM3HAkHCd2A87FOMqOxP_h9xlmG3ks5qJbyugaJpZM4Iek9x
.

@maxpoulin64
Copy link
Member

Yep, I can confirm both issues are fixed. I think the only issue left with this is the hiding of joins and parts. I honestly have no idea how this one can be fixed short of tracking the visibility state in JS :/

@xPaw xPaw added this to the Next Release milestone Jun 13, 2016
@xPaw
Copy link
Member Author

xPaw commented Jun 19, 2016

There's still an annoyance with it. When you open a channel and write a message, it shows the unread marker above your message. This isn't exactly the desired behaviour.

Also, the line is quite an eyesore, need to design something for each theme that doesn't look ass.

@xPaw
Copy link
Member Author

xPaw commented Jun 19, 2016

More fixes!

  • Fixed /clear breaking it
  • The line now moves to the bottom when receiving a self message (e.g. after sending a message)
  • Made the line thinner

I think this is stable enough to make it into 2.0. /cc @thelounge/maintainers

@xPaw xPaw modified the milestones: 2.0.0, Next Release Jun 19, 2016
@williamboman
Copy link
Member

williamboman commented Jun 21, 2016

Seems much smoother now, nice. One thing, when joining a channel with a lot of unread messages the marker is (naturally) placed at the top, but when loading more messages it stays on-top of the previously top-most message.

edit: The issue with it being displayed at the bottom still remains though

@xPaw
Copy link
Member Author

xPaw commented Jun 21, 2016

@williamboman It's mentioned in the OP, and it's not entirely easy to solve due to new bugs it can introduce.

@xPaw
Copy link
Member Author

xPaw commented Jul 4, 2016

@astorije What I mean is, the messages are going to have to move down when the unread marker is moved down, to compensate for lost space. If you want to tweak styles, feel free to commit it yourself.

@astorije
Copy link
Member

astorije commented Jul 4, 2016

@astorije What I mean is, the messages are going to have to move down when the unread marker is moved down, to compensate for lost space.

I see now, and I got to play with it when there is said padding/margin, it didn't shock me. Yes, messages are moved a tiny bit, but I don't see that as an issue.

If you want to tweak styles, feel free to commit it yourself.

I did actually, in a separate commit. I'm pretty happy of the result: it's eye-catchy so you can't miss it, and yet it integrates with the current design (designs actually, tweaked per theme to look even better). Here are the results:

Default Crypto Morning Zenburn Default on mobile
default crypto morning zenburn default mobile

I got inspired from a few apps out there, that got me into specifying the "New messages" text. A few thing:

  • I had to remove that display: table on #chat .messages. I tried many things and didn't see any consequences, both in Chrome and Firefox, but I had to if I wanted to truly center the text. Do you know of any potential consequences?
  • I wanted to place that text in CSS (using content) so it can be easily tweaked per theme and not including in a selection for copy/paste somewhere. I couldn't find a way to avoid the extra span because the outer div already uses :before ... content. If there is a way, I'd be happy to know.
  • I re-used the red color used in different places of the app, plus some transparency on light themes to not make it look aggressive/like an error.

I hope you'll like it 😄

@maxpoulin64
Copy link
Member

Looks pretty good to me @astorije!

For the table, I was sure we introduced it but apparently it comes from e4d6f8f long before we forked.

@astorije
Copy link
Member

astorije commented Jul 4, 2016

Tiny modification between the screenshot and the commit: I extended the margins around the line (to somewhat align with the timestamp) and between the line and "New messages" from 5 to 10px. I'm all set now :)

screen shot 2016-07-04 at 18 27 18

@AlMcKinlay
Copy link
Member

At first glance, it does seem much better than the default colour. I've even removed my colour override, and it seems much more obvious. I'll keep this on for hte day and see how it works. It does seem to have a lot more space around it, and I'm not sure how I feel about that. We'll see how it goes over the day. But absolutely a much better colour (on zenburn).

@astorije astorije force-pushed the xpaw/unread-marker branch 2 times, most recently from e595ea2 to 3c3736f Compare July 5, 2016 14:09
@AlMcKinlay
Copy link
Member

Just to put it here, there was a slight issue with @astorije's changes. If someone had a very long name and you are using zenburn, you would get some really weird formatting issues because the name field was different lengths at different points. It has now been fixed.

@AlMcKinlay
Copy link
Member

Also, it was mentioned on the irc that it might be nice to have the unread marker only disappear on channel change, because the jumping when you send a message is a bit odd. I am not decided what I think about this, yet. I'm kinda of the opinion that it might be better to do that. What do others think?

@williamboman
Copy link
Member

Also, it was mentioned on the irc that it might be nice to have the unread marker only disappear on channel change, because the jumping when you send a message is a bit odd. I am not decided what I think about this, yet. I'm kinda of the opinion that it might be better to do that. What do others think?

I think the way the messages jumps when removing the unread marker is bad UX, so I'm def in favor of this.

@astorije astorije force-pushed the xpaw/unread-marker branch 2 times, most recently from fcf3ca5 to 221e0d6 Compare July 5, 2016 18:54
@dgw
Copy link
Contributor

dgw commented Jul 5, 2016

To illustrate what I'm used to as far as unread marker behavior goes, I'll point at Textual. It does the following:

  • Unread message comes in, channel in focus:
    • Marker stays where it was, visible; it does not hide.
  • Unread message comes in, channel out of focus:
    • If first new unread message since channel lost focus:
      • Marker moves to position just above new message.
    • Else:
      • Marker stays where it was.
  • User sends a new message while unread marker present in channel view:
    • See "Unread message comes in, channel in focus"

I don't think removing the unread marker when the user sends a message looks good. Textual provides a keyboard shortcut and menu option to move the marker to the current position, but that might not be practical/necessary for The Lounge to implement. As much as possible, I think the marker should move while the channel is not visible, to avoid sudden changes in the positions of lines.

@astorije
Copy link
Member

astorije commented Jul 6, 2016

To illustrate what I'm used to as far as unread marker behavior goes, I'll point at Textual. It does the following: [...]

Thanks for the feedback, @dgw, that's helpful!
FYI, current behavior is very much like what Slack and Telegram do. A difference I see is Telegram removes the marker after a few seconds on the focused window, even without sending a message.

I think the way the messages jumps when removing the unread marker is bad UX, so I'm def in favor of this.

How is that bad UX?! Debatable UI, maybe, improvable feature, for sure, but bad UX, definitely not. The alternative offered here means that when sending a message, therefore acknowledging unread messages, the unread messages marker stays, losing its meaning. Assigning different behavior to a component than what it actually is, that becomes bad UX.

That being said, I'm not saying the current behavior/UI/whatever is final, but thousand times better than nothing. As a matter of fact, I can find at least 4 minor things that need improvement. And that's key, here, I think: the feature works very well as is, and apart from the issues reported by @YaManicKill, others are nitpickings that can be resolved later. Well, I know I'll be opening issues on the things that bug me, at least :-)

So, in a nutshell, @YaManicKill, @maxpoulin64, are you still 👍 on this, as a very decent first version?

@maxpoulin64 maxpoulin64 merged commit 18c6152 into master Jul 6, 2016
@maxpoulin64 maxpoulin64 deleted the xpaw/unread-marker branch July 6, 2016 04:06
@astorije astorije self-assigned this Jul 6, 2016
@williamboman
Copy link
Member

williamboman commented Jul 6, 2016

How is that bad UX?!

I meant that the unread marker disappearing itself is not bad UX, but the way an entire chunk of messages repositions themselves while doing so. Creates a momentary disconnect with the content of the chat.

astorije added a commit that referenced this pull request Jul 10, 2016
This re-adds the table layout in CSS removed in 3cddbbc, #332.
@MaxLeiter MaxLeiter mentioned this pull request Oct 10, 2016
matburnham pushed a commit to matburnham/lounge that referenced this pull request Sep 6, 2017
matburnham pushed a commit to matburnham/lounge that referenced this pull request Sep 6, 2017
This re-adds the table layout in CSS removed in 3cddbbc, thelounge#332.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Feature Tickets that describe a desired feature or PRs that add them to the project.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants