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

LB-531: Adjust timestamp format for older listens on listens page #1314

Merged
merged 21 commits into from
Mar 6, 2021
Merged

LB-531: Adjust timestamp format for older listens on listens page #1314

merged 21 commits into from
Mar 6, 2021

Conversation

jdaok
Copy link
Contributor

@jdaok jdaok commented Mar 4, 2021

Jira ticket LB-529:

Problem

The timestamps on the recent listens page can be vague when viewing older listens.

Solution

preciseTimestamp will determine how old the listened_at date is and format it accordingly on the ListenCard:
image (<24 hours)
image (<1 year)
image (>1 year)

@jdaok jdaok changed the title Timestampformat Adjust timestamp format for older listens on listens page Mar 4, 2021
Copy link
Contributor

@MonkeyDo MonkeyDo left a comment

Choose a reason for hiding this comment

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

Thanks for opening this PR, that's a great improvement !

A few of us have been talking about these annoyingly vague timestamps for a little while now, and I just started to try and fix it this week in a WIP feature.
I was going for a similar implementation, but yours is more precise with the optional year, that's great!

We will use this method anywhere we display timestamps; consequently, I'll ask that you move the method to static/js/src/utils.tsx so it can be imported in other components/pages.

@alastair alastair changed the title Adjust timestamp format for older listens on listens page LB-531: Adjust timestamp format for older listens on listens page Mar 4, 2021
@alastair
Copy link
Collaborator

alastair commented Mar 4, 2021

This is super cool, thanks for contributing it! I updated the title so that it references the ticket that we have open to track this task.

As @MonkeyDo said, I really like the year split too.

This is a topic that everyone has an idea about, and there might be some other opinions here, but I think that we should only show "x minutes ago" for listens less than an hour old. Anything else should have a timestamp.

@jdaok
Copy link
Contributor Author

jdaok commented Mar 5, 2021

Thank you for the reviews! :)

This is a topic that everyone has an idea about, and there might be some other opinions here, but I think that we should only show "x minutes ago" for listens less than an hour old. Anything else should have a timestamp.

@alastair I considered this too. Personally since I listen and submit less frequently, they are older and so no "x minutes ago" formatted timestamps would appear on my my recent listens page most of the time.

image

^ Users are still always able to view the exact timestamp for any listen by hovering. (I could also update this to be more similar to the >1 year format for more readability).

I think allowing listens up to 24 hours old (instead of 1 hour old) to be formatted this way makes the page look less formal for users who listen and submit less frequently, but I'd like to get further input as well.

Also updated snapshot and test
Copy link
Contributor

@MonkeyDo MonkeyDo left a comment

Choose a reason for hiding this comment

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

I'm finding more things to tweak as I keep looking at the code, but it's starting to look really nice and much more robust than before ! 🎉

It would be nice to see unit tests for preciseTimestamp where you try throwing curveballs at it to see if you end up with a nice result anyway and nothing breaks.
A unit test file utils.test.tsx was just created yesterday, so you'll need to pull the recent changes on master into your branch.

Keep up the good work, this is an invaluable utility!

listenbrainz/webserver/static/js/src/utils.tsx Outdated Show resolved Hide resolved
Copy link
Contributor

@MonkeyDo MonkeyDo left a comment

Choose a reason for hiding this comment

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

Awesome, thanks a lot !
We're going to use this utility everywhere!

@MonkeyDo MonkeyDo merged commit aad1ebb into metabrainz:master Mar 6, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants