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

feat: Notifications, visual fixes #2884

Open
wants to merge 11 commits into
base: feature/notifications
Choose a base branch
from

Conversation

StaNov
Copy link
Collaborator

@StaNov StaNov commented Jan 29, 2025

No description provided.

@StaNov StaNov requested review from stepan662 and JanCizmar January 29, 2025 13:59
@StaNov StaNov force-pushed the stanov/visual-fixes branch from d3bc103 to a9a51e7 Compare January 29, 2025 14:05
@StaNov StaNov enabled auto-merge (squash) January 29, 2025 14:06
@StaNov StaNov force-pushed the stanov/visual-fixes branch from bfadc3e to 018e172 Compare January 29, 2025 16:32
.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');
}
Copy link
Contributor

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.

Copy link
Contributor

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?

Copy link
Contributor

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.

Copy link
Collaborator Author

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 abbreviate minutes to just min, seconds to s and hours to h.
  • The date-fns component doesn't offer a simple configuration for this (as far as I researched) besides of creating new date-fns-locales 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 or 7d.

obrazek

Copy link
Contributor

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.

Reddit example:
image

Facebook example:

image

@@ -0,0 +1,17 @@
import { useEffect, useState } from 'react';

export function useWindowInnerHeight() {
Copy link
Contributor

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');
}
Copy link
Contributor

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');
}
Copy link
Contributor

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.

@StaNov StaNov self-assigned this Jan 30, 2025
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