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

user-local-time shows empty time for people with no commits #2612

Closed
rahgurung opened this issue Dec 10, 2019 · 13 comments · Fixed by #3525
Closed

user-local-time shows empty time for people with no commits #2612

rahgurung opened this issue Dec 10, 2019 · 13 comments · Fixed by #3525
Labels
change request help wanted small Issues that new contributors can pick up

Comments

@rahgurung
Copy link
Contributor

Maybe a nicer message will help.

Screenshot from 2019-12-10 19-52-58

@rahgurung rahgurung added the bug label Dec 10, 2019
@rahgurung rahgurung changed the title empty time for people with no commits user-local-time - empty time for people with no commits Dec 10, 2019
@rahgurung rahgurung changed the title user-local-time - empty time for people with no commits user-local-time shows empty time for people with no commits Dec 10, 2019
@fregante
Copy link
Member

We do already show this on hover:

@rahgurung
Copy link
Contributor Author

In my system, it comes only when I hover over that time area.

@fregante
Copy link
Member

Right, it's a title attribute

@rahgurung
Copy link
Contributor Author

Will that be enough? How about not adding time part for such users?

@fregante
Copy link
Member

fregante commented Dec 10, 2019

Yeah ideally we wouldn't show the icon at all, but sometimes it might take several seconds until it appears, so we have a "loading" in place. The loading is then replaced by the time or -. Perhaps we could:

  • show Not found, instead
  • check the cache first, if it's there and it's undefined, we shouldn't add the icon at all.

@fregante fregante added change request small Issues that new contributors can pick up help wanted and removed bug labels Dec 10, 2019
@sindresorhus
Copy link
Member

how Not found, instead

👍

@darkred
Copy link
Contributor

darkred commented Aug 30, 2020

I believe that it would be better if no icon nor message was displayed for people with no commits,
i.e. as @fregante described as a different approach:

check the cache first, if it's there and it's undefined, we shouldn't add the icon at all.

I agree with the OP that having a plain clock icon (followed by a - or not) doesn't help,
but I also find the current Not found message distracting, and, at first glance, it always gives me the wrong impression that the relevant account itself no longer exists/has been deleted, instead that's just a RG feature message. e.g.:
2020-08-30_133947.

@yakov116
Copy link
Member

What if we do Timezone not found?

@darkred
Copy link
Contributor

darkred commented Aug 30, 2020

I think it will be a long, distracting string (even if it's in smaller font)
which doesn't offer any info to the user apart from RG debugging
(i.e. that RG is indeed enabled and that it couldn't find the user timezone).
Here is a mockup:
2020-08-30_160831~
I prefer it in its initial form:
2020-08-30_160859~

@fregante
Copy link
Member

fregante commented Aug 30, 2020

If we show the clock icon while loading and then hide it without a reason, it might look like a bug.

If we don't show the clock icon while loading, the user doesn't know whether some information is coming… or if it tried and failed.

"Timezone unknown" would be the more appropriate response.

There could be an additional optimization: if the time is already cached as "missing", then don't show the icon at all.

@darkred
Copy link
Contributor

darkred commented Aug 31, 2020

That's what I also had in mind. Just to summarize:

  • at the initial mouse hover, show the clock icon while loading. Then:
    • if the information has been found, append the found timezone
    • if the information has not been found, append "Timezone unknown" instead
  • at next mouse hover:
    • if the timezone is already cached as value, show the clock icon and the retrieved timezone
    • if the timezone is already cached as "missing", don't show anything at all, not the icon not any text message.

@fregante
Copy link
Member

fregante commented Aug 31, 2020

Code-wise, this could be done with a timer instead of manually checking the cache, etc.

Example to be inserted on line 104

const timeout = delay(300);
const date = getLastCommitDate();
// This will delay adding the icon for 300ms, enough to check if the date is store as "not found"
const race = await Promise.race([timeout, date]);
if (race === false) {
	// `date` was cached as "not found", so don't add the icon at all
	return;
}

The code and comments might be improvable.

@darkred
Copy link
Contributor

darkred commented Sep 4, 2020

Thank you both for fixing this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
change request help wanted small Issues that new contributors can pick up
Development

Successfully merging a pull request may close this issue.

5 participants