Skip to content
This repository has been archived by the owner on Dec 16, 2022. It is now read-only.

Add show timestamp toggle #484

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

floogulinc
Copy link
Collaborator

Closes #93

@astorije
Copy link
Collaborator

astorije commented Oct 7, 2015

Nice addition @floogulinc, thanks!

However, I notice that, on mobile (the smallest version), Shout by defaults hides the timestamp. With your PR, that makes your checkbox appear non working there. I see 2 possible fixes:

  • easy: display timestamp by default on mobile
  • difficult: keep the current default on mobile, but adapt the checkbox and code accordingly

I personally prefer the first solution. Not only it's what I expect on mobile by default (plus it's consistent with other sizes and we can probably merge 2 width categories), but it's also much simpler in code. Solution 2 forces you to check the width of the screen using JS, ... urgh.

What do you think?

@astorije astorije self-assigned this Oct 7, 2015
@floogulinc
Copy link
Collaborator Author

@astorije You can remove timestamps from mobile by removing

#chat .time {
    display: none;
}

from the @media (max-width: 479px) section of the css. But by doing that, messages will take up one extra line:

I honestly don't know if there is enough room on mobile (phones specifically) to do timestamps, at least not in the same way we have them everywhere else.

@astorije
Copy link
Collaborator

astorije commented Oct 7, 2015

Urgh, 2 lines are ugly.

@floogulinc, I am not sure we can make this decision here: some mobiles have gigantic screens, some don't. And fashion goes to increase your screen size as you upgrade your phone, sadly...

My proposal still holds: let's show timestamp by default (first solution) and use your checkbox to please those who would like more room. At the moment, mobile users cannot even display the timestamp, which is very annoying. Your PR will let them (us!) do so while easily switching to no timestamp again.

Thanks! I honestly can't wait for this PR to make it live!

@floogulinc
Copy link
Collaborator Author

@astorije Just note that that the screenshot is from a 6 inch 1440p phone. So it seems pretty ugly on any phone in portrait, If i rotate it then it switches to "tablet" view and works fine with the timestamps.

@richrd
Copy link

richrd commented Oct 7, 2015

+1 again for this. I often miss the timestamps and and have to rotate my device to see them. Kind of sucks. I thinks it would be fine even if the nicks won't line up vertically. I would love this feature!

@astorije
Copy link
Collaborator

astorije commented Oct 8, 2015

@floogulinc, I'm not sure about your meaning, are you agreeing with me?
What your screenshot shows me is that it does look ugly indeed in portrait mode, and we would better off putting everything on the same line everywhere.
Oh and thanks for making me discover that on landscape mode the timestamp appears! How inconsistent :-/

Actually, @richrd has a good point, we do not need to have the timestamps line up on mobile. I think the best result would be to have on mobile (00:00) username message, using :before and :after to add the parentheses.
Would you be OK with that @floogulinc? That would be terrific if you could add that to your PR as it's directly related.

So in a nutshell, here is my final offer:

  • Checkbox to show the timestamp, enabled by default (done)
  • Display the timestamp on mobile by default
  • Surround timestamp on mobile with () and remove alignment between the timestamp and the nick

@astorije
Copy link
Collaborator

@floogulinc, any updates on this? :-)

@astorije
Copy link
Collaborator

astorije commented Jan 6, 2016

@floogulinc, could you make the small change I suggested so that this can go live? It really is a shame we could not finalize this earlier, but I hope we can still do it :-)
If improvements are needed, then fine, but at the slow pace we are going overall, each small step is meaningful.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants