[Feature] iglevels - #1980
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
There was a problem hiding this comment.
Pull request overview
This PR adds level information display to interest groups in the user profile. The feature introduces a new data structure for interest groups that includes selection status and level details (count and unit), along with UI updates to display this information.
Changes:
- Updated interest group data structure to include
selectedflag andlevelobject withcountandunitproperties - Refactored interest group selection/deselection logic to use a toggle pattern instead of add/remove from array
- Enhanced UI to display both karma and level information for each interest group with improved layout
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 7 comments.
| File | Description |
|---|---|
| Profile.tsx | Updated initial interest_groups structure to include selected flag and level object |
| BasicDetails.tsx | Refactored IG selection logic, added level display, and improved UI structure for displaying IG information |
| BasicDetails.module.css | Added styles for new IG text layout and increased width for level display |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| {data.karma > 1000 | ||
| ? (data.karma / 1000).toPrecision(2) + "K" | ||
| : data.karma || "0"} |
There was a problem hiding this comment.
The condition checks data.karma > 1000 but doesn't handle the case where data.karma is null or undefined before the comparison. This could lead to unexpected behavior. The fallback data.karma || "0" only applies when karma is less than or equal to 1000, but if karma is null and greater than 1000 (which shouldn't happen but isn't protected against), it could cause issues.
| {data.karma > 1000 | |
| ? (data.karma / 1000).toPrecision(2) + "K" | |
| : data.karma || "0"} | |
| {data.karma != null && data.karma > 1000 | |
| ? (data.karma / 1000).toPrecision(2) + "K" | |
| : data.karma ?? "0"} |
| ); | ||
| }) | ||
| <p> | ||
| {capitalize(data.level?.unit) || "Level"}{" "} |
There was a problem hiding this comment.
When the fallback for capitalize(data.level?.unit) returns an empty string (when data.level?.unit is falsy), the logical OR will evaluate to "Level", but this creates inconsistent text output. If data.level?.unit is an empty string, capitalize will return an empty string, and the fallback "Level" will be used. However, if the intent is to always show "Level" as the unit type when no unit is provided, this should be more explicit and consistent with the data structure initialization.
| {capitalize(data.level?.unit) || "Level"}{" "} | |
| {capitalize(data.level?.unit ?? "Level")}{" "} |
| )) | ||
| ) : ( | ||
| <p> | ||
| No Interest Groups to Selected, Must be level 4 or above to select |
There was a problem hiding this comment.
Corrected spelling from "No Interest Groups to Selected" to "No Interest Groups Selected".
| No Interest Groups to Selected, Must be level 4 or above to select | |
| No Interest Groups Selected, must be level 4 or above to select |
| if (selectedIg.length < 3) { | ||
| setIg((prev) => [ | ||
| ...prev, | ||
| { ...data, selected: true }, |
There was a problem hiding this comment.
When adding a new interest group from allIg, the code spreads data and adds selected: true. However, if the item from allIg doesn't have a level property with the expected structure (count and unit), this could cause inconsistency with the initialized structure in Profile.tsx. Consider ensuring that added interest groups have a consistent structure that includes the level property with default values.
| { ...data, selected: true }, | |
| { | |
| ...data, | |
| level: { | |
| count: data.level?.count ?? 0, | |
| unit: data.level?.unit ?? "", | |
| }, | |
| selected: true, | |
| }, |
| setIg((prev) => [ | ||
| ...prev, | ||
| { ...data, selected: true }, | ||
| ] | ||
| ); |
There was a problem hiding this comment.
The filter logic checks if an item should be hidden from the "add" list only if it exists in ig with the same name AND is selected. However, when a user deselects an interest group (setting selected: false), that item would reappear in the "add" list even though it already exists in the ig array. If the user then clicks to add it again, it will be pushed to the array again, creating a duplicate entry. The filter should check for name existence only, or the add logic should update the existing entry's selected property to true instead of pushing a new item.
| setIg((prev) => [ | |
| ...prev, | |
| { ...data, selected: true }, | |
| ] | |
| ); | |
| setIg((prev) => { | |
| const exists = prev.some( | |
| (item: any) => item.name === data.name | |
| ); | |
| if (exists) { | |
| return prev.map((item: any) => | |
| item.name === data.name | |
| ? { ...item, selected: true } | |
| : item | |
| ); | |
| } | |
| return [ | |
| ...prev, | |
| { ...data, selected: true }, | |
| ]; | |
| }); |
| ...prev, | ||
| { ...data, selected: true }, | ||
| ] | ||
| ); |
There was a problem hiding this comment.
When a user clicks to add an interest group and selectedIg.length >= 3, the click handler does nothing silently. This provides no feedback to the user about why their action had no effect. Consider adding a toast message to inform the user that they can only select up to 3 interest groups, similar to the error message when trying to remove the last interest group.
| ); | |
| ); | |
| } else { | |
| toast.error( | |
| "You can select up to 3 interest groups only." | |
| ); |
| const ig_sorted = [...ig] | ||
| .filter((ig) => ig.selected) | ||
| .sort((a, b) => (a.name > b.name ? 1 : -1)); | ||
|
|
||
| const selectedIg = [...ig] | ||
| .filter((item) => item.selected) | ||
| .sort((a, b) => a.name.localeCompare(b.name)); |
There was a problem hiding this comment.
The variables ig_sorted and selectedIg are duplicating the same logic - both filter for selected interest groups and sort by name. The only difference is the sorting method (string comparison vs localeCompare), but they produce essentially the same result. Consider removing ig_sorted and using only selectedIg throughout the component, or clarify why both are needed.
…logic with a limit, and refine karma/level display.
refactor: consolidate interest group filtering, enhance IG selection …
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 5 comments.
Comments suppressed due to low confidence (2)
src/modules/Dashboard/modules/Profile/components/BasicDetails/pages/BasicDetails.tsx:124
- Typo in error message. The text should read "No Interest Groups Selected" instead of "No Interest Groups to Selected".
</div>
src/modules/Dashboard/modules/Profile/components/BasicDetails/pages/BasicDetails.tsx:29
- The
capitalizefunction is defined but the casing logic may not work as intended for all inputs. Whentextis an empty string (the default),text.charAt(0)returns an empty string, andtext.slice(1)also returns an empty string, resulting in an empty string being returned. While this works correctly, it would be clearer to explicitly handle the empty string case or add a comment explaining this behavior.
<>
<div className={styles.interestGrp}>
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const ig_sorted = [...ig] | ||
| .filter((ig) => ig.selected) | ||
| .sort((a, b) => a.name.localeCompare(b.name)); | ||
|
|
||
| const capitalize = (text = "") => | ||
| text.charAt(0).toUpperCase() + text.slice(1); | ||
|
|
There was a problem hiding this comment.
The variables ig_sorted and selectedIg both filter and sort the same data in the same way, but use different sorting methods. Line 22 uses a ternary with string comparison (a.name > b.name ? 1 : -1), while line 26 uses localeCompare(). Since ig_sorted is only used once on line 66 and selectedIg is used for rendering, and they produce essentially the same result (just with slightly different sorting algorithms), consider using selectedIg everywhere to avoid redundant computation and inconsistent sorting behavior.
| if (ig.some((item) => item.name === data.name)) { | ||
| setIg((prev) => | ||
| prev.map((item) => | ||
| item.name === data.name | ||
| ? { ...item, selected: true } | ||
| : item | ||
| ) | ||
| ); | ||
| } else { | ||
| setIg((prev) => { | ||
| const exists = prev.some( | ||
| (item: any) => item.name === data.name | ||
| ); | ||
| if (exists) { | ||
| return prev.map((item: any) => | ||
| item.name === data.name | ||
| ? { ...item, selected: true } | ||
| : item | ||
| ); | ||
| } | ||
| return [ | ||
| ...prev, | ||
| { ...data, selected: true }, | ||
| ]; | ||
| }); | ||
| } |
There was a problem hiding this comment.
The filtering logic checks if an item from allIg is present in ig with the same name AND has selected set to true. However, when adding a new interest group (lines 144-148), the item is simply appended to the array. This can lead to duplicate entries if an item was previously deselected and is being added again. The logic should check if the item already exists in the ig array and update its selected property instead of adding a duplicate.
| if (ig.some((item) => item.name === data.name)) { | |
| setIg((prev) => | |
| prev.map((item) => | |
| item.name === data.name | |
| ? { ...item, selected: true } | |
| : item | |
| ) | |
| ); | |
| } else { | |
| setIg((prev) => { | |
| const exists = prev.some( | |
| (item: any) => item.name === data.name | |
| ); | |
| if (exists) { | |
| return prev.map((item: any) => | |
| item.name === data.name | |
| ? { ...item, selected: true } | |
| : item | |
| ); | |
| } | |
| return [ | |
| ...prev, | |
| { ...data, selected: true }, | |
| ]; | |
| }); | |
| } | |
| setIg((prev) => { | |
| const exists = prev.some( | |
| (item: any) => item.name === data.name | |
| ); | |
| if (exists) { | |
| return prev.map((item: any) => | |
| item.name === data.name | |
| ? { ...item, selected: true } | |
| : item | |
| ); | |
| } | |
| return [ | |
| ...prev, | |
| { ...data, selected: true }, | |
| ]; | |
| }); |
| </div> | ||
| ); | ||
| }) | ||
| <p> | ||
| {capitalize(data.level.unit)}{" "} |
There was a problem hiding this comment.
The karma display logic is missing null check before comparison. The expression data.karma > 1000 will evaluate to false when data.karma is null or undefined, then fallback to data.karma || "0" which works. However, this should be explicitly handled for clarity, similar to how the level data is handled with optional chaining.
There was a problem hiding this comment.
@copilot open a new pull request to apply changes based on this feedback
| )) | ||
| ) : ( |
There was a problem hiding this comment.
The fallback logic for the level display is inconsistent. Line 117 has || "Level" as fallback when data.level?.unit is falsy, but line 118 has ?? 1 as fallback when data.level?.count is null or undefined. The first should use ?? instead of || to be consistent, or both should use ||. Using ?? (nullish coalescing) is more precise as it only falls back on null/undefined, whereas || falls back on any falsy value including 0 or empty string.
No description provided.