feat: implement notification list component #2009
Conversation
|
Allure Report
processReports: ✅ test report for 3a954b67
|
There was a problem hiding this comment.
The NotificationListItem shows the topic rather than the publisher (or the chain, I have not clear from figma).
What this PR is missing is that clicking on the NotificationListItem the user should be brought to the notification center with the desired notification information available (even if the rendering of the details is not ready yet).
| const formatNotificationCount = (count: number) => (count < 10 ? count.toString() : '9+'); | ||
| const MAX_NOTIFICATION_COUNT = 9; | ||
| const formatNotificationCount = (count: number) => | ||
| count < MAX_NOTIFICATION_COUNT ? count.toString() : `${MAX_NOTIFICATION_COUNT}+`; |
There was a problem hiding this comment.
| count < MAX_NOTIFICATION_COUNT ? count.toString() : `${MAX_NOTIFICATION_COUNT}+`; | |
| count <= MAX_NOTIFICATION_COUNT ? count.toString() : `${MAX_NOTIFICATION_COUNT}+`; |
| import styles from './NotificationsBell.module.scss'; | ||
|
|
||
| import NotificationBellIcon from '@lace/core/src/ui/assets/icons/notifications-bell.component.svg'; | ||
| import NotificationBellIcon from '../../assets/icons/notifications-bell.component.svg'; |
There was a problem hiding this comment.
@mirceahasegan asked me to put icons in the core package.
Anyway if there are good reasons to keep them in this package, please remove the other ones (also the happy face).
| import { useHistory } from 'react-router'; | ||
| import { SectionTitle } from '@components/Layout/SectionTitle'; | ||
|
|
||
| export const NotificationsCenter = (): React.ReactElement => { |
There was a problem hiding this comment.
Two component with the same name and purpose?
I'd suggest a refactor to have only one component with that purpose.
| remove | ||
| remove, | ||
| // TODO: Implement loadMore and isLoading | ||
| loadMore: () => { |
There was a problem hiding this comment.
At UI level loading is a synchronous action. The SW provides all the notifications at once.
I'd simplify the entire PR removing every reference to dynamic loading.
| [notifications] | ||
| ); | ||
|
|
||
| const mappedNotifications = useMemo( |
There was a problem hiding this comment.
I do not understand this mapping. Apart from flattening the properties at root level it basically filters out some properties.
I see only two disadvantages in having this (more resources required and the need to modify this in case UI needs some property removed here) and no advantages.
Also the I'd say that implementing the onClick should be part of this PR, but in the NotificationListItem directly rather than here.
| return ( | ||
| <Flex flexDirection="column" alignItems="center" justifyContent="center" gap="$8"> | ||
| <HappyFaceIcon className={styles.icon} /> | ||
| <Text.Body.Large>{t('notificationsCenter.allClear.title')}</Text.Body.Large> |
There was a problem hiding this comment.
Please also remove the allClear.title and allClear.description properties from the translation file.
| /> | ||
| ); | ||
| }; | ||
| export const NotificationsCenterContainer = (): React.ReactElement => <NotificationsCenter />; |
There was a problem hiding this comment.
@mirceahasegan asked me for keeping the logic in this component and passing hooks to the inner component.
I'm more inclined to the proposed approach, but for sure we need some more coherence:
- either stick to @mirceahasegan directives
- or remove this now useless component
Checklist
Proposed solution
Explain how does this PR solves the problem stated in JIRA ticket.
You can also enumerate different alternatives considered while approaching this task.
Testing
Describe here, how the new implementation can be tested.
Provide link or briefly describe User Acceptance Criteria/Tests that need to be met
Screenshots