-
Notifications
You must be signed in to change notification settings - Fork 0
Implement notification setup with tests #49
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,9 +1,15 @@ | ||
| /** | ||
| * Placeholder function for initializing notifications. | ||
| * TODO: Implement actual notification initialization logic. | ||
| * Initialize the notification system by creating a toast container | ||
| * if it doesn't already exist. This container will hold all | ||
| * notification elements. | ||
| */ | ||
| function initializeNotifications() { | ||
| console.warn('initializeNotifications function called, but not implemented.'); | ||
| let container = document.querySelector('.notification-container'); | ||
| if (!container) { | ||
| container = document.createElement('div'); | ||
| container.className = 'notification-container'; | ||
| document.body.appendChild(container); | ||
| } | ||
| } | ||
| /** | ||
| * Vivid Market JavaScript | ||
|
|
@@ -217,26 +223,31 @@ function initializeItemPreviews() { | |
| * @param {string} type - Notification type (success, error, info) | ||
| */ | ||
| function showNotification(message, type = 'info') { | ||
| // Create notification element if it doesn't exist | ||
| let notification = document.querySelector('.notification'); | ||
| if (!notification) { | ||
| notification = document.createElement('div'); | ||
| notification.className = 'notification'; | ||
| document.body.appendChild(notification); | ||
| } | ||
| // Ensure container exists | ||
| initializeNotifications(); | ||
|
|
||
| // Set notification content and type | ||
| notification.textContent = message; | ||
| const container = document.querySelector('.notification-container'); | ||
|
|
||
| // Create individual toast element | ||
| const notification = document.createElement('div'); | ||
| notification.className = `notification ${type}`; | ||
| notification.textContent = message; | ||
|
|
||
| container.appendChild(notification); | ||
|
|
||
| // Show notification | ||
| // Trigger show animation | ||
| setTimeout(() => { | ||
| notification.classList.add('show'); | ||
| }, 10); | ||
|
|
||
| // Hide notification after 3 seconds | ||
| // Hide and remove after 3 seconds | ||
| setTimeout(() => { | ||
| notification.classList.remove('show'); | ||
| notification.addEventListener( | ||
| 'transitionend', | ||
| () => notification.remove(), | ||
| { once: true } | ||
| ); | ||
| }, 3000); | ||
| } | ||
|
|
||
|
Comment on lines
223
to
253
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The Recommendation: Implement error handling to check if elements are successfully found or created before proceeding with further actions. Consider consolidating or managing timeouts more efficiently to prevent potential memory leaks or performance issues. |
||
|
|
@@ -377,3 +388,12 @@ function initializePurchaseFlow() { | |
| function isUserLoggedIn() { | ||
| return document.body.classList.contains('user-logged-in'); | ||
| } | ||
|
|
||
| // Export functions for testing in Node environments | ||
| if (typeof module !== 'undefined') { | ||
| module.exports = { | ||
| initializeNotifications, | ||
| showNotification, | ||
| isUserLoggedIn | ||
| }; | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,34 @@ | ||
| /** | ||
| * @jest-environment jsdom | ||
| */ | ||
| const { initializeNotifications, showNotification } = require('../public/js/vivid-market'); | ||
|
|
||
| describe('Vivid Market Notifications', () => { | ||
| beforeEach(() => { | ||
| document.body.innerHTML = ''; | ||
| }); | ||
|
|
||
| test('initializeNotifications creates a container', () => { | ||
| initializeNotifications(); | ||
| const container = document.querySelector('.notification-container'); | ||
| expect(container).not.toBeNull(); | ||
| }); | ||
|
|
||
| test('showNotification appends a toast', () => { | ||
| jest.useFakeTimers(); | ||
| initializeNotifications(); | ||
| showNotification('Test', 'success'); | ||
|
|
||
| const container = document.querySelector('.notification-container'); | ||
| expect(container.children.length).toBe(1); | ||
|
|
||
| const toast = container.firstElementChild; | ||
| expect(toast.textContent).toBe('Test'); | ||
| expect(toast.classList.contains('success')).toBe(true); | ||
|
|
||
| jest.advanceTimersByTime(20); | ||
| expect(toast.classList.contains('show')).toBe(true); | ||
|
|
||
| jest.useRealTimers(); | ||
|
Comment on lines
+18
to
+32
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The handling of fake timers in the test 'showNotification appends a toast' is potentially problematic. The timers are set to fake with Recommendation: |
||
| }); | ||
| }); | ||
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 function
initializeNotificationsdirectly manipulates the DOM to ensure a notification container exists. While this is functional, it could lead to inefficiencies if called multiple times, as each call involves querying and potentially modifying the DOM.Recommendation: Consider checking and initializing the notification container once and reusing it, or using a more efficient method to manage DOM updates, such as using a JavaScript framework that handles the DOM more efficiently.