-
-
Notifications
You must be signed in to change notification settings - Fork 221
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
feat: Notifications, visual fixes #2884
base: feature/notifications
Are you sure you want to change the base?
Conversation
d3bc103
to
a9a51e7
Compare
…nted by the notifications
bfadc3e
to
018e172
Compare
.replaceAll(/\b(\d+?)\b ([Mm])inut.*?\b/g, '$1 $2in') | ||
.replaceAll(/\b(\d+?)\b hodin.*?\b/g, '$1 hod') | ||
.replaceAll(/\b(\d+?)\b (sekund|second|segund).*?\b/g, '$1 s'); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, this doesn't look very clean. Is it necessary? If we keep it there should be some comment about it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I hope this is some kind of joke. Can you please explain what it does?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Anyway, this has to very probably have to go as separate translations with proper pluralization.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your comments, guys.
The reasoning is this:
- The requirement from design was to show the age of the notification.
- We are already using the
date-fns
component at other places, so I decided to use it also here. - After another design iteration, it was found that
15 minutes ago
is too long and it would be better to abbreviateminutes
to justmin
,seconds
tos
andhours
toh
. - The
date-fns
component doesn't offer a simple configuration for this (as far as I researched) besides of creating newdate-fns-locale
s for this case. It is supplied with two dates and a locale and it returns a localized string. - So my low-level effort to at least prototype it to see it on the screen was this - take the output string and shorten the long words to the abbreviated ones.
- After another design iteration, it might be better to drop using the localized text and go for just
15m
or7d
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am for making it simplier...
It doesn't read well. Classic meme issue with reading row first.
I would remove all the not-necessary information.
- we don't need the language there (the user probably knows which languages they know)
- the user don't need to know task number
- the user probably doesn't need the project name, because there is usually single project
- Actually, instead of showing the user avatar, I would show the project avatar, if it's project event. I am not that interested about the information who assigned me. (usually it will be my single manager)
Anyway, they will find out when they open it.
The notifications should not replicate the function of activity log. Notifications only notify. It's role is not to inform about everything.
The time can be super short format. Like facebook and reddit have.
Facebook example:
@@ -0,0 +1,17 @@ | |||
import { useEffect, useState } from 'react'; | |||
|
|||
export function useWindowInnerHeight() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can use
import { useWindowSize } from 'usehooks-ts';
instead of this
.replaceAll(/\b(\d+?)\b ([Mm])inut.*?\b/g, '$1 $2in') | ||
.replaceAll(/\b(\d+?)\b hodin.*?\b/g, '$1 hod') | ||
.replaceAll(/\b(\d+?)\b (sekund|second|segund).*?\b/g, '$1 s'); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I hope this is some kind of joke. Can you please explain what it does?
.replaceAll(/\b(\d+?)\b ([Mm])inut.*?\b/g, '$1 $2in') | ||
.replaceAll(/\b(\d+?)\b hodin.*?\b/g, '$1 hod') | ||
.replaceAll(/\b(\d+?)\b (sekund|second|segund).*?\b/g, '$1 s'); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Anyway, this has to very probably have to go as separate translations with proper pluralization.
No description provided.