-
Notifications
You must be signed in to change notification settings - Fork 13
feat: implement notification list component #2009
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
Conversation
Allure Report
processReports: ✅ test report for 36c9f455
|
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.
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).
apps/browser-extension-wallet/src/components/NotificationsCenter/NotificationsBell.tsx
Outdated
Show resolved
Hide resolved
apps/browser-extension-wallet/src/features/notifications-center/NotificationsBell.tsx
Show resolved
Hide resolved
import { useHistory } from 'react-router'; | ||
import { SectionTitle } from '@components/Layout/SectionTitle'; | ||
|
||
export const NotificationsCenter = (): React.ReactElement => { |
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.
Two component with the same name and purpose?
I'd suggest a refactor to have only one component with that purpose.
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.
- moved all the components into the features folder
- move browser view vs popup view specific container components into the dedicated view folders
- added details page template
done here 87356f9
apps/browser-extension-wallet/src/hooks/useNotificationsCenter.ts
Outdated
Show resolved
Hide resolved
apps/browser-extension-wallet/src/hooks/useNotificationsCenter.ts
Outdated
Show resolved
Hide resolved
apps/browser-extension-wallet/src/components/NotificationsCenter/NotificationsAllClear.tsx
Show resolved
Hide resolved
...browser-extension-wallet/src/components/NotificationsCenter/NotificationsCenterContainer.tsx
Outdated
Show resolved
Hide resolved
aeae32f
to
87356f9
Compare
|
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.
Excellent! ❤️
87356f9
to
36c9f45
Compare
|
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
Details page template: