Bug: Fixing a profile dropdown behavior and added a chevron-down icon#948
Conversation
|
Hey @tejaskh3 can you please review it now? |
|
Hello @ZendeAditya please check if you have push the code changes.
|
I haven't pushed the code yet because I'm not able to understand what are you saying about in the test case. |
ZendeAditya
changed the title
WalkthroughThe changes implement a new feature flag based on the URL parameter Changes
Sequence Diagram(s)sequenceDiagram
participant User as User
participant Browser as Browser
participant Navbar as Navbar Module
participant Dropdown as Dropdown Element
User->>Browser: Navigate with URL?dev=true/false
Browser->>Navbar: Initialize navbar
Navbar->>Navbar: Check URL parameters (devFlag)
alt devFlag true
Navbar->>Navbar: Render chevron icon
Navbar->>Browser: Attach outside click listener
else
Navbar->>Navbar: Render navbar without chevron
end
User->>Browser: Click outside dropdown
Browser->>Navbar: Dispatch click event
alt devFlag true & dropdown active
Navbar->>Dropdown: Remove 'active' class (close dropdown)
else
Navbar->>Dropdown: Maintain dropdown state
end
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
__tests__/home/home.test.js (1)
560-590: Test case verifies dropdown closing behavior correctly.The test case successfully verifies that clicking outside the dropdown closes it. A few minor improvements:
- Consider using optional chaining in the dropdown element references (lines 573 and 582)
- For more realistic testing, consider triggering the dropdown through user interaction rather than directly manipulating the DOM
- const isActive = await page.evaluate(() => { - const dropdown = document.getElementById('dropdown'); - return dropdown && dropdown.classList.contains('active'); - }); + const isActive = await page.evaluate(() => { + const dropdown = document.getElementById('dropdown'); + return dropdown?.classList.contains('active'); + }); - const isActiveAfterClick = await page.evaluate(() => { - const dropdown = document.getElementById('dropdown'); - return dropdown && dropdown.classList.contains('active'); - }); + const isActiveAfterClick = await page.evaluate(() => { + const dropdown = document.getElementById('dropdown'); + return dropdown?.classList.contains('active'); + });🧰 Tools
🪛 Biome (1.9.4)
[error] 573-573: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 582-582: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
images/chevron-down.svgis excluded by!**/*.svg
📒 Files selected for processing (4)
__tests__/home/home.test.js(2 hunks)__tests__/navbar/navbar.test.js(1 hunks)navbar.global.js(1 hunks)userLogin.js(1 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
__tests__/home/home.test.js
[error] 573-573: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 582-582: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🔇 Additional comments (4)
__tests__/navbar/navbar.test.js (1)
18-20: Good addition to the navbar test coverage.The test for the chevron icon is well structured and follows the established pattern of the existing element checks in this test file.
userLogin.js (1)
92-100: Great improvement to user experience!This event listener properly closes the dropdown when a user clicks outside both the dropdown menu and the user info element, which is an important usability feature.
navbar.global.js (2)
34-36: Chevron icon implementation looks good.The chevron down icon is correctly implemented using the SVG image as recommended in the previous feedback.
38-38: Good addition of data-testid attribute.Adding the data-testid attribute improves the testability of this component.
|
Please update the branch! |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
__tests__/navbar/navbar.test.js (1)
126-149: Test verifies dropdown remains open in non-development mode.This test is important for ensuring that the dropdown behavior is mode-dependent, staying open when clicking outside in non-development mode. Consider improving the code by using optional chaining for better null safety.
- let dropdownIsActive = await page.evaluate(() => { - const dropdown = document.getElementById('dropdown'); - return dropdown && dropdown.classList.contains('active'); - }); + let dropdownIsActive = await page.evaluate(() => { + const dropdown = document.getElementById('dropdown'); + return dropdown?.classList.contains('active') || false; + });🧰 Tools
🪛 Biome (1.9.4)
[error] 137-137: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
__tests__/home/home.test.js(1 hunks)__tests__/navbar/navbar.test.js(2 hunks)navbar.global.js(1 hunks)userLogin.js(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- userLogin.js
- tests/home/home.test.js
- navbar.global.js
🧰 Additional context used
🪛 Biome (1.9.4)
__tests__/navbar/navbar.test.js
[error] 137-137: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🔇 Additional comments (2)
__tests__/navbar/navbar.test.js (2)
18-20: LGTM: Clear test for the new chevron icon.The addition of this check to the
testNavbarfunction aligns with the PR objective of adding a chevron-down icon to the user profile section.
105-125: Test confirms dropdown closes when clicking outside in development mode.This test effectively verifies the main behavior enhancement described in the PR: that the dropdown menu closes when clicking anywhere outside the user profile area when in development mode.
|
Hello @ZendeAditya can you please update the video as you have added the dev flag? |
Yes |
ZendeAditya
force-pushed
the
bug/profile-dropdown
branch
from
fbd7ab6 to
2c89f29
Compare
|
Hey @AnujChhikara please check now |
ZendeAditya
force-pushed
the
bug/profile-dropdown
branch
from
c92f05b to
df37986
Compare
|
Hello @ZendeAditya, |
Yes. |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
__tests__/navbar/navbar.test.js (2)
18-30: Use if-else structure for the devFlag conditions.The current implementation uses two separate if conditions (
if (devFlag)andif (!devFlag)) which are mutually exclusive. This can be simplified to an if-else structure for better readability and efficiency.if (devFlag) { const chevronIcon = await navbarPage.$('#chevron-down'); expect(chevronIcon).toBeTruthy(); -} -if (!devFlag) { +} else { const chevronIcon = await navbarPage.$('#chevron-down'); expect(chevronIcon).toBeFalsy(); }
124-129: Use Puppeteer's API for element interactions instead of page.evaluate.For better reliability and readability, use Puppeteer's built-in methods for clicking elements instead of executing JavaScript in the page context.
- await page.evaluate(() => { - const userInfo = document.querySelector('.user-info'); - if (userInfo) { - userInfo.click(); - } - }); + await userInfoHandle.click();
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
__tests__/navbar/navbar.test.js(2 hunks)
🔇 Additional comments (2)
__tests__/navbar/navbar.test.js (2)
115-135: Use test data attributes instead of direct CSS selectors.As mentioned in a previous review comment, you should use the test-id attributes that were added to these elements instead of CSS selectors for better test stability.
- const userInfoHandle = await page.$('.user-info'); - const dropdownHandle = await page.$('#dropdown'); + const userInfoHandle = await page.$('[data-testid="user-info"]'); + const dropdownHandle = await page.$('[data-testid="dropdown"]');Also, verify that the dropdown is open before attempting to close it to make the test more robust:
await page.evaluate(() => { const userInfo = document.querySelector('.user-info'); if (userInfo) { userInfo.click(); } }); + // Wait for dropdown to be active + await page.waitForFunction(() => { + const dropdown = document.getElementById('dropdown'); + return dropdown?.classList.contains('active'); + }); await page.mouse.click(100, 100);
145-148: Good use of null-safe checking.The null-safe approach with optional chaining and nullish coalescing operator is a good practice. This follows the previous review suggestion and makes the code more robust.
ZendeAditya
force-pushed
the
bug/profile-dropdown
branch
from
b83a78e to
84eccfa
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
__tests__/navbar/navbar.test.js (2)
24-24: Simplify the boolean expression.The ternary operator here is unnecessary since
devFlagis already a boolean.- expect(chevronIcon).toBe(devFlag ? true : false); + expect(chevronIcon).toBe(devFlag);🧰 Tools
🪛 Biome (1.9.4)
[error] 24-24: Unnecessary use of boolean literals in conditional expression.
Simplify your code by directly assigning the result without using a ternary operator.
If your goal is negation, you may use the logical NOT (!) or double NOT (!!) operator for clearer and concise code.
Check for more details about NOT operator.
Unsafe fix: Remove the conditional expression with(lint/complexity/noUselessTernary)
118-123: Simplify user interaction with Puppeteer's API.Rather than using page.evaluate for clicking an element, use Puppeteer's built-in click method.
- await page.evaluate(() => { - const userInfo = document.querySelector('.user-info'); - if (userInfo) { - userInfo.click(); - } - }); + await userInfoHandle.click();
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
__tests__/navbar/navbar.test.js(2 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
__tests__/navbar/navbar.test.js
[error] 24-24: Unnecessary use of boolean literals in conditional expression.
Simplify your code by directly assigning the result without using a ternary operator.
If your goal is negation, you may use the logical NOT (!) or double NOT (!!) operator for clearer and concise code.
Check for more details about NOT operator.
Unsafe fix: Remove the conditional expression with
(lint/complexity/noUselessTernary)
🔇 Additional comments (5)
__tests__/navbar/navbar.test.js (5)
113-114: Use test-ids for element selection.Using CSS selectors directly makes tests brittle. Previous review comments already flagged this issue.
- const userInfoHandle = await page.$('.user-info'); - const dropdownHandle = await page.$('#dropdown'); + const userInfoHandle = await page.$('[data-testid="user-info"]'); + const dropdownHandle = await page.$('[data-testid="dropdown"]');
133-138: Test actual user interaction instead of manually modifying the DOM.Instead of manually adding the 'active' class to the dropdown, simulate the actual user interaction that would open the dropdown for a more realistic test.
- await page.evaluate(() => { - const dropdown = document.getElementById('dropdown'); - if (dropdown && !dropdown.classList.contains('active')) { - dropdown.classList.add('active'); - } - }); + const userInfoHandle = await page.$('.user-info'); + expect(userInfoHandle).toBeTruthy(); + await userInfoHandle.click(); + await page.waitForFunction( + () => document.querySelector('#dropdown').classList.contains('active'), + { timeout: 1000 } + );
139-142: Simplify dropdown active state check.Use the suggested approach from previous reviews for checking if dropdown is active.
- let dropdownIsActive = await page.evaluate(() => { - const dropdown = document.getElementById('dropdown'); - return dropdown?.classList.contains('active') ?? false; - }); + const dropdownHandle = await page.$('#dropdown'); + const dropdownIsActive = await dropdownHandle.evaluate( + (el) => el.classList.contains('active') + );
144-147: Avoid using document.body.click() and arbitrary timeouts.Using
document.body.click()may not accurately simulate a user clicking outside the dropdown. Use Puppeteer'spage.mouse.click()method instead.- await page.evaluate(() => { - document.body.click(); - }); - await page.waitForTimeout(200); + await page.mouse.click(10, 10); // Click at a point clearly outside the dropdown + // Wait for any potential state changes to occur + await page.waitForFunction(() => true, { timeout: 100 });
130-153: Use consistent approach for testing dropdown behavior.The second test uses a different approach for testing compared to the first test. For consistency and to simulate real user behavior, adopt the same approach in both tests.
Consider restructuring the second test to match the pattern of the first test, focusing only on changing the feature flag value:
it('should keep the dropdown open when clicking outside when feature flag is off', async () => { await page.goto(`${LOCAL_TEST_PAGE_URL}?dev=false`); const userInfoHandle = await page.$('.user-info'); const dropdownHandle = await page.$('#dropdown'); expect(userInfoHandle).toBeTruthy(); expect(dropdownHandle).toBeTruthy(); // Click to open dropdown await userInfoHandle.click(); // Verify dropdown is open const isDropdownActive = await dropdownHandle.evaluate( (el) => el.classList.contains('active') ); expect(isDropdownActive).toBe(true); // Click outside dropdown await page.mouse.click(100, 100); // Verify dropdown is still open (since dev=false) const dropdownStillActive = await dropdownHandle.evaluate( (el) => el.classList.contains('active') ); expect(dropdownStillActive).toBe(true); });
__tests__/navbar/navbar.test.js
Outdated
| }); | ||
|
|
||
| const chevronIcon = await navbarPage.$('#chevron-down'); | ||
| expect(chevronIcon).toBe(devFlag ? true : false); |
There was a problem hiding this comment.
| expect(chevronIcon).toBe(devFlag ? true : false); | |
| expect(chevronIcon).toBe(devFlag); |
There was a problem hiding this comment.
Added an if else block without it if chevron icon not present chevronIcon return null instead of true or false.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
__tests__/navbar/navbar.test.js (2)
113-133: 🛠️ Refactor suggestionImprove dropdown test by using test IDs and consistent interaction patterns.
The test should use test IDs and maintain a consistent approach for user interactions.
Implement these improvements:
- Use the suggested test name format from previous comments
- it('should close the dropdown by clicking outside the dropdown under dev feature flag', async () => { + it('should close the dropdown by clicking outside the dropdown when dev feature flag is on', async () => {
- Use test IDs instead of class selectors:
- const userInfoHandle = await page.$('.user-info'); + const userInfoHandle = await page.$('[data-testid="user-info"]');
- Use a more consistent approach for user interaction - don't mix page.evaluate() and direct mouse clicks:
- await page.evaluate(() => { - const userInfo = document.querySelector('.user-info'); - if (userInfo) { - userInfo.click(); - } - }); + await userInfoHandle.click(); + // Give time for the dropdown to appear + await page.waitForFunction(() => document.querySelector('#dropdown').classList.contains('active'), { timeout: 1000 });
- Make the assertion more concise:
- const dropdownIsActive = await dropdownHandle.evaluate((el) => - el.classList.contains('active'), - ); - expect(dropdownIsActive).toBe(false); + expect(await dropdownHandle.evaluate(el => el.classList.contains('active'))).toBe(false);
134-157: 🛠️ Refactor suggestionUse consistent approaches for testing dropdown behavior.
There are several issues with this test that were pointed out in previous reviews but haven't been fully addressed.
- Use the suggested test name format:
- it('should keep the dropdown open when clicking outside when feature flag is off', async () => { + it('should keep the dropdown open when clicking outside when dev feature flag is off', async () => {
- Use consistent approach for interacting with the dropdown:
await page.goto(`${LOCAL_TEST_PAGE_URL}?dev=false`); await page.waitForSelector('#dropdown'); - await page.evaluate(() => { - const dropdown = document.getElementById('dropdown'); - if (dropdown && !dropdown.classList.contains('active')) { - dropdown.classList.add('active'); - } - }); + const userInfoHandle = await page.$('.user-info'); + expect(userInfoHandle).toBeTruthy(); + await userInfoHandle.click(); + await page.waitForFunction(() => document.querySelector('#dropdown').classList.contains('active'), { timeout: 1000 });
- Replace document.body.click() and arbitrary timeouts:
- await page.evaluate(() => { - document.body.click(); - }); - await page.waitForTimeout(200); + await page.mouse.click(10, 10); // Click at a point clearly outside the dropdown + // Wait a moment for any potential state changes + await page.waitForFunction(() => true, { timeout: 100 });
- Make the assertion style consistent with the first test:
- const newDropdownHandle = await page.$('#dropdown'); - const newDropdownIsActive = await newDropdownHandle.evaluate((el) => - el.classList.contains('active'), - ); - expect(newDropdownIsActive).toBe(true); + const dropdownHandle = await page.$('#dropdown'); + expect(await dropdownHandle.evaluate(el => el.classList.contains('active'))).toBe(true);
🧹 Nitpick comments (1)
__tests__/navbar/navbar.test.js (1)
19-28: Simplify the conditional chevron icon assertion.The conditional logic for checking the chevron icon presence can be made more concise, as suggested in a previous review.
const devFlag = await navbarPage.evaluate(() => { return new URLSearchParams(window.location.search).get('dev') === 'true'; }); const chevronIcon = await navbarPage.$('#chevron-down'); - if (devFlag) { - expect(chevronIcon).toBeTruthy(); - } else { - expect(chevronIcon).toBeFalsy(); - } + expect(Boolean(chevronIcon)).toBe(devFlag);
ZendeAditya
force-pushed
the
bug/profile-dropdown
branch
from
423b98f to
760ee37
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
requests/util.js (1)
142-153: Consider making the container selection more flexible.The
addRadioButtonfunction hardcodes the container ID to'filterOptionsContainer'. Making this a parameter would increase reusability across different parts of the application.-function addRadioButton(labelText, value, groupName) { - const group = document.getElementById('filterOptionsContainer'); +function addRadioButton(labelText, value, groupName, containerId = 'filterOptionsContainer') { + const group = document.getElementById(containerId);task-requests/details/script.js (1)
717-720: Consider using CSS classes instead of inline styles.Using inline styles makes maintenance more difficult. Consider moving these styles to CSS classes:
- descriptionValue.style.border = 'none'; - descriptionValue.style.marginTop = '-0.35rem'; - descriptionValue.style.padding = '-0rem 0.7rem'; + descriptionValue.classList.add('description-value-no-border');Then define the CSS class in your stylesheet.
requests/script.js (1)
183-184: Consider using a constant instead of repeating string comparisonThe repeated string comparison could be replaced with a boolean variable for better readability.
- const isRequestTypeOnboarding = type === ONBOARDING_EXTENSION_REQUEST_TYPE; + // Define this near the top of your function + const isRequestTypeOnboarding = type === ONBOARDING_EXTENSION_REQUEST_TYPE;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
images/chevron-down.svgis excluded by!**/*.svg
📒 Files selected for processing (16)
__tests__/navbar/navbar.test.js(2 hunks)__tests__/requests/requests.test.js(4 hunks)__tests__/task-requests/task-requestDetails.test.js(3 hunks)global.css(0 hunks)mock-data/requests/index.js(1 hunks)mock-data/taskRequests/index.js(3 hunks)navbar.global.js(2 hunks)requests/constants.js(2 hunks)requests/index.html(2 hunks)requests/script.js(21 hunks)requests/style.css(7 hunks)requests/util.js(3 hunks)style.css(1 hunks)task-requests/details/script.js(3 hunks)task-requests/details/style.css(5 hunks)userLogin.js(2 hunks)
💤 Files with no reviewable changes (1)
- global.css
🚧 Files skipped from review as they are similar to previous changes (3)
- navbar.global.js
- userLogin.js
- tests/navbar/navbar.test.js
🔇 Additional comments (68)
style.css (1)
242-243: Responsive design improvement for small screens.The addition of
height: auto;andmargin-top: 3rem;enhances the button section's display on mobile devices, allowing it to properly resize based on content while maintaining adequate spacing.mock-data/requests/index.js (2)
76-110: Mock data added for onboarding extension requests.The new
onboardingExtensionRequestconstant provides necessary test data for both approved and pending onboarding request states, following the same structure as other request types in the file.
117-117: Exported the new mock data constant.Properly exported the
onboardingExtensionRequestconstant to make it available for imports in test files.mock-data/taskRequests/index.js (3)
3-6: Added comprehensive task description for testing.This new
longDescriptionconstant provides a detailed, realistic task description that better represents actual task requirements, which is valuable for thorough testing.
62-62: Updated task description reference.Changed the hard-coded description to use the new
longDescriptionconstant, which provides more realistic test data.
393-393: Exported longDescription.Added the new constant to module exports, making it available for use in test files.
requests/constants.js (3)
10-10: Added new request type for onboarding extensions.New constant
ONBOARDING_EXTENSION_REQUEST_TYPEfollows the existing naming pattern and supports the onboarding extension feature described in the PR objectives.
14-14: Added tab ID for onboarding extension section.The new
ONBOARDING_EXTENSION_TAB_IDconstant is consistent with existing tab ID naming conventions and will be used for DOM selection in the UI.
29-29: Added error message for onboarding extension requests.This error message follows the existing pattern for handling request not found scenarios, improving error handling consistency.
__tests__/task-requests/task-requestDetails.test.js (5)
11-11: Good addition of imported longDescription.Using an imported constant instead of a hardcoded string improves maintainability.
148-148: Update assertion to use the imported constant.This change correctly updates the assertion to use the imported
longDescriptionvariable instead of a hardcoded string, making the test more maintainable.
162-179: Well-structured test for modal rendering with dev flag enabled.This new test verifies that the new modal elements are correctly rendered when the dev flag is enabled. Good use of specific element selectors and expectations.
181-198: Good complementary test for dev flag disabled case.This test provides balanced coverage by verifying the old modal is rendered when the dev flag is disabled. Great approach to test both scenarios.
200-222: Comprehensive test for long description handling.This test thoroughly checks how long descriptions are handled in the modal, including:
- Verifying description length exceeds 1000 characters
- Checking modal scrollability for overflow content
Good attention to user experience details.
requests/util.js (2)
27-28: Good enhancement to support testId attribute.Adding support for the
testIdattribute improves testability by allowing elements to be easily identified in tests.
37-47: Well-structured query parameter function.This new function consolidates query parameter handling into a single reusable function. Good approach to reduce code duplication.
__tests__/requests/requests.test.js (6)
7-7: Good addition of onboarding extension request mock.Adding the mock data import supports the new tests for onboarding extension functionality.
70-82: Well-structured request interception for onboarding extension requests.The intercepted request handling is consistent with other request types in the test.
131-134: Improved test stability with waitForFunction.Using
waitForFunctionto wait for the status text to change to 'Approved' makes the test more reliable by ensuring the UI has updated before checking the assertion.
161-171: Good test for conditional visibility based on dev flag.The tests verify that elements are conditionally displayed based on the dev flag, which aligns with the PR objectives.
173-247: Comprehensive test suite for onboarding requests UI.These tests thoroughly verify different aspects of the onboarding requests UI in dev mode:
- Tab visibility
- Request card rendering
- Conditional display of action buttons
- Correct status display
Well-structured and comprehensive.
277-372: Thorough test coverage for filter functionality.The tests cover all key aspects of the filter functionality:
- Visibility of filter container and elements
- Toggle behavior of the filter modal
- Rendering of filter modal elements
- Closing behavior with clear and apply buttons
- Initial filter state
This provides excellent coverage for this feature.
task-requests/details/script.js (2)
684-690: Good conditional text handling with fallback.Using a fallback value ('N/A') for null or undefined descriptions in dev mode improves the user experience by avoiding empty display.
701-761: Well-structured conditional modal content rendering.The code now properly differentiates between dev and non-dev rendering approaches:
- In dev mode, it creates a more structured layout with separated divs
- In non-dev mode, it maintains the original implementation
This aligns with the PR objective to improve dropdown behavior.
requests/script.js (15)
11-20: Good implementation of new filter-related variablesThe variables are clearly named and align with their corresponding HTML elements. This facilitates the implementation of the filter functionality.
35-40: Appropriate conditional rendering based on development flagThe code correctly shows/hides the onboarding extension tab and modifies container classes based on the
isDevflag, allowing for feature toggling during development.
62-70: Remember to include proper event preventionGood implementation of event prevention with
event.preventDefault(), which prevents default anchor behavior while handling the tab switch programmatically.
75-86: Tab switching logic is well-implementedThe code correctly toggles CSS classes to highlight the active tab while removing highlight from others, including the new onboarding extension tab.
88-99: Consistent implementation of the onboarding extension tab handlerThe new event listener follows the same pattern as the existing ones, ensuring consistent behavior across different tabs.
107-115: Good consolidation of request handling logicThe
getRequestsfunction effectively replaces separate functions for different request types, making the code more maintainable. The error message selection based on request type is a good practice.
132-133: Improved error handling with dynamic error messagesUsing a variable to determine the error message based on request type improves user experience with contextual feedback.
203-210: Good conditional date field handlingThe code correctly displays appropriate date fields based on the request type, using the
isRequestTypeOnboardingflag to determine which dates to display.
334-335: Ensure consistent message handling across request typesThe logic correctly displays different content based on request type, improving user experience.
351-353: Clear presentation of reason vs. message fieldsGood implementation of displaying the correct text based on request type, improving clarity for users.
457-459: Robust user details retrieval with type checkingThe code properly handles different user ID fields based on request type, ensuring correct user information is displayed.
600-612: Well-implemented filter toggle with outside click handlingThe filter toggle and outside click handler follow best practices for modal interactions, ensuring the filter can be opened and closed appropriately.
614-622: Escape key handling improves accessibilityAdding keyboard support for closing the filter modal with the Escape key is a good accessibility practice.
624-629: Event propagation properly managedUsing
event.stopPropagation()correctly prevents conflicts between the filter button click and document click handlers.
631-663: Well-structured filter population functionThe
populateStatusfunction creates a clean UI structure with appropriate event handlers. The clear button implementation is particularly useful for resetting filters.task-requests/details/style.css (10)
12-12: Good addition of indigo-blue color variableAdding the new color variable to the root helps maintain consistency across the application.
460-475: Well-structured modal content stylingThe new modal content class provides clear positioning and styling that will ensure the modal displays correctly, with appropriate dimensions and overflow handling.
526-533: Consistent close button stylingThe close button styling matches the application's overall design pattern while providing clear visual affordance for closing the modal.
549-555: Clear heading style for improved readabilityThe modal heading style makes good use of the new indigo-blue color and provides appropriate text alignment and weight.
557-562: Well-organized content divisionThe styling for date and description divs provides consistent spacing and organization of modal content.
563-568: Consistent text styling for labelsThe text styling for labels maintains consistency with appropriate font weight for visual hierarchy.
570-575: Subtle value styling for better readabilityThe styling for values creates a clear visual distinction between labels and values through color.
577-584: Consistent heading styles within descriptionStandardizing the heading styles within the description ensures consistent presentation of content.
602-625: Responsive design for different screen sizesGood implementation of media queries to adjust the layout for smaller screens, ensuring the UI remains usable.
640-663: Thoughtful mobile-specific stylingThe mobile-specific adjustments to font sizes and spacing ensure the modal remains readable on smaller screens.
requests/style.css (14)
4-6: Updated color variables to solid hex valuesChanging from semi-transparent RGBA to solid hex colors provides more consistent visual appearance across the application.
108-135: Well-structured user suggestion componentsThe styling for user suggestions provides a clean dropdown experience with appropriate hover effects and containment.
137-153: Clear filter option stylingThe filter options container and clear button are well styled, with appropriate hover effects that indicate interactivity.
155-160: Consistent filter container layoutThe filter container uses flexbox appropriately to create a clean layout with proper spacing.
162-168: Properly styled search inputThe search input styling aligns with the application's design language, with appropriate sizing and border radius.
177-188: Clear and accessible filter toggle buttonThe filter toggle button has good contrast and padding, making it easy to identify and click.
190-214: Comprehensive modal stylingThe filter modal styling provides appropriate layering, shadows, and padding for a good user experience.
216-238: Good styling for filter checkboxes and buttonsThe styling ensures filter options are clearly presented with appropriate spacing and emphasis.
245-255: Flexible request container layoutThe new container layout allows for better organization of request cards in a column layout when needed.
262-267: Improved request card sizingSetting a maximum width on request cards prevents them from becoming too wide on larger screens, improving readability.
305-311: Typography improvements for better readabilityThe updated font size and weight for requester information creates better visual hierarchy.
359-362: Consistent timeline text stylingThe improved font weight for timeline text enhances readability of important date information.
422-424: Typography consistency improvementsUpdating the font weight for admin information creates consistency across different text elements.
686-689: Responsive filter containerThe media query adjustments ensure the filter container displays properly on smaller screens.
requests/index.html (5)
37-50: Good implementation of the onboarding tab with proper test attributesThe new onboarding tab is correctly structured with appropriate classes and test attributes. The "hidden" class ensures it's only visible when needed.
51-63: Consistent test attribute addition for existing tabsAdding test attributes to existing tabs improves testability while maintaining consistent structure.
71-98: Well-structured filter container with user suggestion componentsThe filter container includes all necessary elements for user searching with appropriate test attributes and initial hidden state.
100-138: Clean implementation of filter modalThe filter toggle button and modal are properly structured with all necessary elements and test attributes.
141-145: Request container test attribute additionAdding the test attribute to the request container improves testability while maintaining existing functionality.
ZendeAditya
force-pushed
the
bug/profile-dropdown
branch
2 times, most recently
from
2ae5217 to
a8f294e
Compare
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 5
🔭 Outside diff range comments (1)
requests/script.js (1)
88-99: 🛠️ Refactor suggestionTab handler code is duplicated.
The event listener code for tab navigation is duplicated across three similar blocks. This violates the DRY principle and increases the chance of bugs if one version is updated but others aren't.
Consider refactoring to use a single generic function:
+function handleTabClick(activeTab, inactiveTabs, requestType) { + return async function(event) { + event.preventDefault(); + if (isDataLoading) return; + currentReqType = requestType; + nextLink = ''; + activeTab.classList.add(selected__tab__class); + inactiveTabs.forEach(tab => tab.classList.remove(selected__tab__class)); + changeFilter(); + updateUrlWithQuery(currentReqType); + await renderRequestCards({ state: statusValue, sort: sortByValue }); + }; +} + +oooTabLink.addEventListener('click', handleTabClick( + oooTabLink, + [extensionTabLink, onboardingExtensionTabLink], + OOO_REQUEST_TYPE +)); + +extensionTabLink.addEventListener('click', handleTabClick( + extensionTabLink, + [oooTabLink, onboardingExtensionTabLink], + EXTENSION_REQUEST_TYPE +)); + +onboardingExtensionTabLink.addEventListener('click', handleTabClick( + onboardingExtensionTabLink, + [oooTabLink, extensionTabLink], + ONBOARDING_EXTENSION_REQUEST_TYPE +));
🧹 Nitpick comments (11)
requests/util.js (1)
142-153: Consider making the radio button function more reusable.The
addRadioButtonfunction could be more flexible by accepting a container parameter instead of hardcoding the container ID.-function addRadioButton(labelText, value, groupName) { - const group = document.getElementById('filterOptionsContainer'); +function addRadioButton(labelText, value, groupName, containerId = 'filterOptionsContainer') { + const group = document.getElementById(containerId);requests/script.js (7)
11-13: Good element selection, but consider centralizing DOM references.The approach of caching DOM elements is good for performance, but these selectors are scattered throughout the code. Consider grouping all DOM element selections at the top of the file for better maintainability.
35-40: Feature flag implementation needs improvement.The
isDevcheck conditionally shows UI elements, but this pattern is duplicated elsewhere in the code. Consider creating a single function to handle feature-flagged UI elements for better maintainability.-if (isDev) { - onboardingExtensionTabLink.classList.remove('hidden'); - requestContainer.classList.remove('request'); - requestContainer.classList.add('request_container'); - filterContainer.classList.remove('hidden'); -} +function setupDevFeaturesUI() { + if (isDev) { + onboardingExtensionTabLink.classList.remove('hidden'); + requestContainer.classList.remove('request'); + requestContainer.classList.add('request_container'); + filterContainer.classList.remove('hidden'); + } +} + +setupDevFeaturesUI();
107-115: Consolidating request functions is good, but error handling needs improvement.Consolidating the request functions with a
requestTypeparameter is a good improvement, but the error message selection could be more robust.Consider using an object map instead of conditional logic:
- const notFoundErrorMessage = - requestType === ONBOARDING_EXTENSION_REQUEST_TYPE - ? ErrorMessages.ONBOARDING_EXTENSION_NOT_FOUND - : ErrorMessages.OOO_NOT_FOUND; + const notFoundErrorMessages = { + [ONBOARDING_EXTENSION_REQUEST_TYPE]: ErrorMessages.ONBOARDING_EXTENSION_NOT_FOUND, + [OOO_REQUEST_TYPE]: ErrorMessages.OOO_NOT_FOUND, + [EXTENSION_REQUEST_TYPE]: ErrorMessages.OOO_NOT_FOUND + }; + const notFoundErrorMessage = notFoundErrorMessages[requestType] || ErrorMessages.OOO_NOT_FOUND;
204-210: Conditional date handling is clear but could be more DRY.The code handles different date fields based on request type effectively, but repeating the condition is not DRY.
- const fromDate = convertDateToReadableStringDate( - isRequestTypeOnboarding ? oldEndsOn : from, - DEFAULT_DATE_FORMAT, - ); - const toDate = convertDateToReadableStringDate( - isRequestTypeOnboarding ? newEndsOn : until, - DEFAULT_DATE_FORMAT, - ); + const dateFields = isRequestTypeOnboarding + ? { fromField: oldEndsOn, toField: newEndsOn } + : { fromField: from, toField: until }; + + const fromDate = convertDateToReadableStringDate( + dateFields.fromField, + DEFAULT_DATE_FORMAT, + ); + const toDate = convertDateToReadableStringDate( + dateFields.toField, + DEFAULT_DATE_FORMAT, + );
457-459: Avoid nested awaits for better readability.The code uses an await inside a variable assignment with a ternary operator, which can be difficult to follow.
- let requesterUserDetails = await getUserDetails( - request.type === 'OOO' ? request.requestedBy : request.userId, - ); + const userId = request.type === 'OOO' ? request.requestedBy : request.userId; + let requesterUserDetails = await getUserDetails(userId);
600-622: Filter toggle implementation needs improvement.The filter toggle and close functions work but could be improved. The toggle function should be more explicit, and there's no visual indication when the filter is active.
function toggleFilter() { - filterModal.classList.toggle('hidden'); + if (filterModal.classList.contains('hidden')) { + openFilter(); + } else { + closeFilter(); + } } +function openFilter() { + filterModal.classList.remove('hidden'); + filterButton.classList.add('active'); // Add an active class for visual feedback +} function closeFilter() { filterModal.classList.add('hidden'); + filterButton.classList.remove('active'); }
631-665: Status filter creation could be more maintainable.The
populateStatusfunction creates DOM elements imperatively, which is harder to maintain than a template-based approach.Consider using HTML templates or a more declarative approach:
function populateStatus() { const statusList = [ { name: 'Approved', id: 'APPROVED' }, { name: 'Pending', id: 'PENDING' }, { name: 'Rejected', id: 'REJECTED' }, ]; - const filterHeader = document.createElement('div'); - filterHeader.className = 'filter__header'; - - const filterTitle = document.createElement('p'); - filterTitle.className = 'filter__title'; - filterTitle.textContent = 'Filter By Status'; - - const clearButton = document.createElement('button'); - clearButton.className = 'filter__clear__button'; - clearButton.textContent = 'Clear'; - clearButton.setAttribute('data-testid', 'filter-clear-button'); - - filterHeader.append(filterTitle); - - clearButton.addEventListener('click', async function () { - filterModal.classList.add('hidden'); - requestContainer.innerHTML = ''; - await renderRequestCards({ state: statusValue, sort: sortByValue }); - }); - - filterTitle.appendChild(clearButton); - document.querySelector('#filterOptionsContainer').prepend(filterHeader); + const headerTemplate = ` + <div class="filter__header"> + <p class="filter__title"> + Filter By Status + <button class="filter__clear__button" data-testid="filter-clear-button">Clear</button> + </p> + </div> + `; + + filterOptionsContainer.insertAdjacentHTML('afterbegin', headerTemplate); + + const clearButton = filterOptionsContainer.querySelector('.filter__clear__button'); + clearButton.addEventListener('click', async function () { + filterModal.classList.add('hidden'); + requestContainer.innerHTML = ''; + await renderRequestCards({ state: statusValue, sort: sortByValue }); + }); for (const { name, id } of statusList) { addRadioButton(name, id, 'status-filter'); } }task-requests/details/style.css (3)
12-12: Good addition of a color variable, but document its usage.Adding a new color variable is good practice for maintainability, but consider adding a comment to document where and why this color is used.
- --color-indigo-blue: #5145cd; + /* Used for modal headings and interactive elements in dev mode */ + --color-indigo-blue: #5145cd;
526-533: Good styling for close button, but consider accessibility improvements.The close button styling is good, but it should have additional accessibility attributes for screen readers.
While this is CSS, ensure the corresponding HTML element has:
<button class="new_requestor_details_modal_close" aria-label="Close modal" type="button">×</button>
602-625: Media query implementation is good but could be more comprehensive.The media query handles layout changes well but doesn't address all potential responsive issues.
Consider adding more granular breakpoints or using a mobile-first approach:
@media (max-width: 904px) { .task { grid-column: 1 / span 12; } .requestors { grid-column: 1 / span 12; border: none; } .taskRequest__skeleton__title { max-width: 80%; height: 1rem; } .taskRequest__skeleton__subtitle { max-width: 40%; } .request-details { grid-column: auto; } .new_requestor_details_modal_content { width: 70%; } + /* Add styles for other potentially problematic elements at this breakpoint */ + .container { + padding: 1.5rem; + } }
🛑 Comments failed to post (5)
requests/script.js (2)
604-618: 🛠️ Refactor suggestion
Document event listeners could cause memory leaks.
Adding document-level event listeners without cleanup can lead to memory leaks if this code runs multiple times.
Consider using a single event handler and adding it once during initialization:
+// Define these at the top of the file with other initializations +function handleDocumentClick(event) { + if ( + !filterModal.classList.contains('hidden') && + !filterModal.contains(event.target) && + event.target !== filterButton + ) { + closeFilter(); + } +} + +function handleDocumentKeydown(event) { + if (event.key === 'Escape' && !filterModal.classList.contains('hidden')) { + closeFilter(); + } +} + +// Add this to an initialization function +document.addEventListener('click', handleDocumentClick); +document.addEventListener('keydown', handleDocumentKeydown);Committable suggestion skipped: line range outside the PR's diff.
624-629: 🛠️ Refactor suggestion
Filter buttons need accessibility attributes.
The filter buttons lack proper accessibility attributes, making them difficult to use for screen reader users.
-filterButton.addEventListener('click', (event) => { - event.stopPropagation(); - toggleFilter(); -}); +filterButton.setAttribute('aria-haspopup', 'true'); +filterButton.setAttribute('aria-expanded', 'false'); +filterButton.addEventListener('click', (event) => { + event.stopPropagation(); + toggleFilter(); + // Update aria-expanded based on filter visibility + filterButton.setAttribute('aria-expanded', !filterModal.classList.contains('hidden')); +});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.filterButton.setAttribute('aria-haspopup', 'true'); filterButton.setAttribute('aria-expanded', 'false'); filterButton.addEventListener('click', (event) => { event.stopPropagation(); toggleFilter(); // Update aria-expanded based on filter visibility filterButton.setAttribute('aria-expanded', !filterModal.classList.contains('hidden')); }); applyFilterButton.addEventListener('click', closeFilter);task-requests/details/style.css (1)
640-664: 🛠️ Refactor suggestion
Font sizes at small breakpoints may cause accessibility issues.
The font sizes in the smallest breakpoint (0.75rem) may be too small for some users, potentially causing accessibility issues.
- .proposed_start_date_text, - .proposed_end_date_text, - .proposed_description_text, - .requestor_description_details :where(h1, h2, h3, h4, h5, h6) { - font-size: 0.75rem; - } + /* Minimum font size of 0.875rem (14px) is recommended for accessibility */ + .proposed_start_date_text, + .proposed_end_date_text, + .proposed_description_text, + .requestor_description_details :where(h1, h2, h3, h4, h5, h6) { + font-size: 0.875rem; + }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements..new_requestor_details_modal_heading { font-size: 1.05rem; margin-top: -0.6rem; } /* Minimum font size of 0.875rem (14px) is recommended for accessibility */ .proposed_start_date_text, .proposed_end_date_text, .proposed_description_text, .requestor_description_details :where(h1, h2, h3, h4, h5, h6) { font-size: 0.875rem; } .proposed_start_date_value, .proposed_end_date_value, .proposed_description_value, .requestor_description_details :not(:where(h1, h2, h3, h5, h6)) { font-size: 0.75rem; } .new_requestor_details_modal_content { height: 40%; } .new_requestor_details_modal_close { font-size: 0.85rem; } }requests/style.css (2)
108-135: 🛠️ Refactor suggestion
Well-structured user suggestions styles, but missing focus states.
The user suggestions component styles are well organized, but there's no defined style for keyboard focus states, which is an accessibility issue.
.suggestion { padding: 0.5rem; cursor: pointer; width: 8rem; border-bottom: solid 1px var(--color-gray); &:hover { background-color: var(--color-gray-light); } + &:focus-visible { + outline: 2px solid var(--color-primary); + outline-offset: -2px; + background-color: var(--color-gray-light); + } }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements..user__suggestions__container { display: flex; flex-direction: column; } .suggestions__list__container { max-height: 6.25rem; overflow-y: auto; position: absolute; margin-top: 2.5rem; } .user__suggestions__list { border: 1px solid var(--color-gray); border-bottom: none; background-color: var(--white); border-radius: 0.25rem; } .suggestion { padding: 0.5rem; cursor: pointer; width: 8rem; border-bottom: solid 1px var(--color-gray); &:hover { background-color: var(--color-gray-light); } &:focus-visible { outline: 2px solid var(--color-primary); outline-offset: -2px; background-color: var(--color-gray-light); } }
177-238: 🛠️ Refactor suggestion
Filter button and modal styles need accessibility improvements.
The filter button and modal styles look good visually, but need accessibility improvements for keyboard navigation and screen readers.
.filter__toggle__button { display: flex; justify-content: space-around; width: 8rem; align-items: center; font-size: 1rem; background-color: var(--color-primary); border-radius: 0.25rem; border: none; cursor: pointer; padding: 0.4rem 1.5rem; + color: var(--white); /* Missing text color */ + &:focus-visible { + outline: 2px solid var(--white); + outline-offset: 2px; + } } .filter__modal { border: var(--base-light-grey-border); padding: 1rem; margin: 3rem 0; position: absolute; z-index: 1; background-color: var(--white); box-shadow: var(--elevation-3); border-radius: 0.5rem; display: flex; flex-direction: column; gap: 1rem; font-size: 0.9rem; + /* Add right position to ensure it appears in a predictable location */ + right: 0; }Committable suggestion skipped: line range outside the PR's diff.
ZendeAditya
force-pushed
the
bug/profile-dropdown
branch
from
a8f294e to
fb12c83
Compare
ZendeAditya
force-pushed
the
bug/profile-dropdown
branch
from
fb12c83 to
6a76498
Compare
|
Hey @AnujChhikara @MayankBansal12 Please take a look now |
6 participants
Date: 20/02/2025
Developer Name: @ZendeAditya
Issue Ticket Number
Description
To add a chevron down icon to the right side of the user profile and when the profile dropdown is open and the user clicks anywhere on the page except the user profile area the dropdown will close.
Chevron-Down Icon Addition: A chevron-down icon has been added to the right side of the user profile section. This icon is designed to be responsive, ensuring it displays correctly across all screen sizes.
Dropdown Behavior Improvement: The dropdown menu associated with the user profile will now close when the user clicks anywhere on the page, provided the click occurs outside the user profile area (userInfo) or the dropdown itself. This enhancement improves the user experience by allowing for a more intuitive interaction with the dropdown menu.
Documentation Updated?
Under Feature Flag
Database Changes
Breaking Changes
Development Tested?
Screenshots
Screenshot 1
loom-video.6.mp4
Test Coverage
Screenshot 1
Additional Notes
Summary by CodeRabbit
New Features
Tests