PM-3357 - show profile completeness banner #402
Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| } = $ctx.auth) | ||
|
|
||
| let toast: ToastType | undefined; | ||
| let banner: {key: string, date: Date} | undefined; |
There was a problem hiding this comment.
[maintainability]
Consider defining a type for the banner object to improve type safety and maintainability. This will make it easier to understand the structure of the banner object and prevent potential errors.
| <Banner | ||
| userHandle={user?.handle ?? ''} | ||
| banner={banner} | ||
| redirect={toastsMeta[banner.key].ctaLink(user?.handle ?? '')} |
There was a problem hiding this comment.
[❗❗ correctness]
Ensure that toastsMeta[banner.key] is always defined before accessing ctaLink. If banner.key is not present in toastsMeta, this could lead to a runtime error.
| return getCookieValue(COOKIE_NAME); | ||
| } | ||
|
|
||
| const isOlderThanTreshold = (date: Date | number, treshold: number): boolean => { |
There was a problem hiding this comment.
[💡 readability]
The function name isOlderThanTreshold contains a typo in Treshold. It should be Threshold for clarity and consistency.
| return; | ||
| } | ||
|
|
||
| const lastUpdateOrCofirmDate = lastProfileConfirmationDate ? Math.max(lastUpdate, +lastProfileConfirmationDate) : lastUpdate; |
There was a problem hiding this comment.
[💡 readability]
The variable name lastUpdateOrCofirmDate contains a typo in Cofirm. It should be Confirm for clarity and consistency.
| .sort((a, b) => +a[1] - +b[1]); | ||
|
|
||
|
|
||
| const lastUpdate = +sorted[sorted.length - 1][1]; |
There was a problem hiding this comment.
[❗❗ correctness]
Accessing sorted[sorted.length - 1] without checking if sorted is empty could lead to an error. Consider adding a check to ensure sorted has elements before accessing.
| } | ||
| } | ||
|
|
||
| return JSON.parse(lastSeen); |
There was a problem hiding this comment.
[❗❗ correctness]
The function getBanner returns the result of JSON.parse(lastSeen) without checking if lastSeen is a valid JSON string. Consider adding error handling for JSON.parse to prevent runtime errors.
| pointer-events: none; | ||
| // move up and fade in when shown | ||
| transform: translate(0, -30px); | ||
| transition: transform 0.3s ease-in-out, opacity 0.18s ease-in-out 0.1s, visibility 0.01ms 0.3s; |
There was a problem hiding this comment.
[correctness]
The visibility transition duration is set to 0.01ms, which is effectively instantaneous and may not have the intended effect. Consider removing it or setting it to a more meaningful duration.
| pointer-events: all; | ||
| // move down and fade out when hidden | ||
| transform: translate(0, 0); | ||
| transition: transform 0.3s ease-in-out, opacity 0.18s ease-in-out, visibility 0.01ms; |
There was a problem hiding this comment.
[correctness]
The visibility transition duration is set to 0.01ms, which is effectively instantaneous and may not have the intended effect. Consider removing it or setting it to a more meaningful duration.
| border-radius: 0.25rem; | ||
|
|
||
| transition: background 0.15s ease; | ||
| cursor: pointer; |
There was a problem hiding this comment.
[💡 maintainability]
The cursor: pointer; property is declared twice in the .inlineBtn class. Consider removing the duplicate declaration to improve maintainability.
|
|
||
| function dismiss() { | ||
| visible = false; | ||
| setTimeout(dispatch, 300, 'dismiss'); |
There was a problem hiding this comment.
[correctness]
Using setTimeout with dispatch may lead to unexpected behavior if the component is unmounted before the timeout completes. Consider using a more reliable method to handle the dispatch, such as checking if the component is still mounted.
| } | ||
|
|
||
| async function confirm() { | ||
| await confirmProfileData(userHandle) |
There was a problem hiding this comment.
[❗❗ correctness]
The confirmProfileData function is awaited, but there is no error handling. Consider adding a try-catch block to handle potential errors during the confirmation process.
| return `${day}${suffix} ${months[d.getMonth()]} ${d.getFullYear()}`; | ||
| } | ||
|
|
||
| onMount(() => { |
There was a problem hiding this comment.
[💡 performance]
The setTimeout function is used to set visible to true after 15 milliseconds. This might be unnecessary and could be replaced with a more direct approach unless there is a specific reason for the delay.
| </div> | ||
| {:else} | ||
| <div class={styles.contents}> | ||
| <a href={redirectUrl} target="_blank"> |
There was a problem hiding this comment.
[❗❗ security]
The anchor tag uses target="_blank" without rel="noopener noreferrer". This can be a security risk as it allows the new page to access the window object of the page that opened it. Consider adding rel="noopener noreferrer" to mitigate this risk.
|
|
||
| <div class={styles.spacer} /> | ||
| <a | ||
| href={redirectUrl} |
There was a problem hiding this comment.
[❗❗ security]
The anchor tag uses target="_blank" without rel="noopener noreferrer". This can be a security risk as it allows the new page to access the window object of the page that opened it. Consider adding rel="noopener noreferrer" to mitigate this risk.
| background: #D4D4D4; | ||
| width: 1px; | ||
| height: 60px; | ||
| height: var(--uninav-header-height); |
There was a problem hiding this comment.
[maintainability]
Using a CSS variable var(--uninav-header-height) for the height is a good practice for maintainability and consistency. Ensure that this variable is defined and has a fallback value to prevent potential rendering issues if the variable is not set.
| instancesContextStore[targetId] = ctx; | ||
|
|
||
| if (navType === 'tool' || navType === 'marketing') { | ||
| await loadNudgeApp(ctx, targetEl as Element); |
There was a problem hiding this comment.
[performance]
The await keyword is used with loadNudgeApp, but it was not used in the previous implementation. Ensure that loadNudgeApp is intended to be asynchronous and that this change does not introduce unintended side effects, such as delaying the loading of the navigation component.
| handle: string; | ||
| percentComplete: number; | ||
| showToast: string; | ||
| dateFields?: [string, Date][]; |
There was a problem hiding this comment.
[💡 readability]
Consider using a more descriptive type for dateFields instead of [string, Date][]. This will improve readability and maintainability by making it clear what each element of the tuple represents. For example, you could define a type alias like type DateField = [fieldName: string, lastUpdated: Date]; and use DateField[].
| data: { | ||
| percentComplete: number; | ||
| } & { | ||
| [key: string]: any; |
There was a problem hiding this comment.
[maintainability]
Using [key: string]: any in the ProfileCompletednessResponse interface can lead to potential type safety issues. Consider defining specific keys or using a more precise type instead of any to improve type safety and maintainability.
Added banners to act as periodic reminders for members to keep their profile up to date.
UI: