Conversation
…teness PM-3357 - show profile completeness banner
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| @@ -0,0 +1,3 @@ | |||
| <svg xmlns="http://www.w3.org/2000/svg" fill="none" viewBox="0 0 24 24" stroke-width="1.5" stroke="white" class="size-6"> | |||
There was a problem hiding this comment.
[maintainability]
Consider specifying a width and height attribute on the <svg> element to ensure consistent rendering across different environments and prevent layout shifts.
| @@ -0,0 +1,3 @@ | |||
| <svg xmlns="http://www.w3.org/2000/svg" fill="none" viewBox="0 0 24 24" stroke-width="1.5" stroke="white" class="size-6"> | |||
There was a problem hiding this comment.
[design]
The stroke="white" attribute may cause visibility issues if the SVG is used on a light background. Consider using a more flexible approach, such as CSS, to control the stroke color based on the context where the SVG is used.
| @@ -0,0 +1,3 @@ | |||
| <svg xmlns="http://www.w3.org/2000/svg" fill="none" viewBox="0 0 24 24" stroke-width="1.5" stroke="white" class="size-6"> | |||
There was a problem hiding this comment.
[💡 maintainability]
Consider using a more descriptive class name than size-6 to improve maintainability and clarity. This will help future developers understand the purpose of the class without needing to refer to external stylesheets.
| @@ -0,0 +1,3 @@ | |||
| <svg xmlns="http://www.w3.org/2000/svg" fill="none" viewBox="0 0 24 24" stroke-width="1.5" stroke="white" class="size-6"> | |||
There was a problem hiding this comment.
[correctness]
The stroke="white" attribute may not be visible against a light background. Ensure that the SVG is used in a context where the stroke color provides sufficient contrast for accessibility.
| @@ -0,0 +1 @@ | |||
| <svg width="16" height="16" viewBox="0 0 16 16" fill="none" xmlns="http://www.w3.org/2000/svg"><path d="M5.3 11.75L8 9.05L10.7 11.75L11.75 10.7L9.05 8L11.75 5.3L10.7 4.25L8 6.95L5.3 4.25L4.25 5.3L6.95 8L4.25 10.7L5.3 11.75ZM8 15.5C6.9625 15.5 5.9875 15.3031 5.075 14.9094C4.1625 14.5156 3.36875 13.9813 2.69375 13.3063C2.01875 12.6313 1.48438 11.8375 1.09063 10.925C0.696875 10.0125 0.5 9.0375 0.5 8C0.5 6.9625 0.696875 5.9875 1.09063 5.075C1.48438 4.1625 2.01875 3.36875 2.69375 2.69375C3.36875 2.01875 4.1625 1.48438 5.075 1.09063C5.9875 0.696875 6.9625 0.5 8 0.5C9.0375 0.5 10.0125 0.696875 10.925 1.09063C11.8375 1.48438 12.6313 2.01875 13.3063 2.69375C13.9813 3.36875 14.5156 4.1625 14.9094 5.075C15.3031 5.9875 15.5 6.9625 15.5 8C15.5 9.0375 15.3031 10.0125 14.9094 10.925C14.5156 11.8375 13.9813 12.6313 13.3063 13.3063C12.6313 13.9813 11.8375 14.5156 10.925 14.9094C10.0125 15.3031 9.0375 15.5 8 15.5ZM8 14C9.675 14 11.0938 13.4187 12.2563 12.2563C13.4187 11.0938 14 9.675 14 8C14 6.325 13.4187 4.90625 12.2563 3.74375C11.0938 2.58125 9.675 2 8 2C6.325 2 4.90625 2.58125 3.74375 3.74375C2.58125 4.90625 2 6.325 2 8C2 9.675 2.58125 11.0938 3.74375 12.2563C4.90625 13.4187 6.325 14 8 14Z" fill="white"></path></svg> | |||
There was a problem hiding this comment.
[accessibility]
Consider adding a title or aria-label attribute to the <svg> element to improve accessibility for screen readers.
| handle: string | ||
| percentComplete: number | ||
| showToast: string | ||
| dateFields?: [string, Date][] |
There was a problem hiding this comment.
[maintainability]
The type dateFields?: [string, Date][] suggests an array of tuples, each containing a string and a Date. Consider using a more descriptive type alias or interface for the tuple to improve readability and maintainability, especially if this structure is used in multiple places.
| const isFixed = (scrollY + yOffset - elYOffset) >= 0; | ||
| const isFixed = (scrollY + yOffset - elYOffset - delayYOffset) >= 0; | ||
| if (placeholderRef) { | ||
| Object.assign(placeholderRef.style, {height: isFixed ? `${elRef.offsetHeight}px` : 0}); |
There was a problem hiding this comment.
[maintainability]
Using Object.assign to modify the style object directly can lead to unexpected behavior if other styles are applied dynamically elsewhere. Consider using style.height = ... for clarity and to avoid potential side effects.
| }) | ||
| </script> | ||
|
|
||
| <div bind:this={placeholderRef}></div> |
There was a problem hiding this comment.
[design]
The placeholder <div> is being used to maintain layout when the sticky element is fixed. Ensure that this placeholder does not interfere with other layout elements or cause unexpected shifts in the layout.
| completed: completednessData.data?.percentComplete === 100, | ||
| handle: completednessData.handle, | ||
| percentComplete: completednessData.data?.percentComplete, | ||
| showToast: DISABLE_NUDGES ? '' : completednessData.showToast, |
There was a problem hiding this comment.
[💡 maintainability]
The use of a ternary operator to set showToast based on DISABLE_NUDGES is correct, but it might be more maintainable to handle this logic outside of the object literal for clarity, especially if more conditions are added in the future.
|
|
||
| export function isOnHost(host: string): boolean { | ||
| const locationHostname = window?.location.hostname ?? '' | ||
| return !!host.match(new RegExp(`^https?:\/\/${locationHostname}`, 'i')); |
There was a problem hiding this comment.
[❗❗ correctness]
The regular expression in isOnHost does not correctly match the host. The ^https?:\/\/ pattern is unnecessary and incorrect for matching hostnames. Consider using ^${locationHostname}$ to match the hostname exactly.
| (!!NUDGES_DISABLED_HOSTS.find(host => ( | ||
| host.match(new RegExp(`^https?:\/\/${locationHostname}`, 'i')) | ||
| ))); | ||
| return DISABLE_NUDGES || NUDGES_DISABLED_HOSTS.filter(h => !exceptHost || h !== exceptHost).some(isOnHost); |
There was a problem hiding this comment.
[performance]
Using filter followed by some in dismissNudgesBasedOnHost can be inefficient. Consider using some directly with a combined condition to improve performance.
| const responseData = response.data ?? {}; | ||
| const dateFields = Object.keys(responseData) | ||
| .filter(k => k.endsWith('LastUpdateDate') || k === 'lastProfileConfirmationDate') | ||
| .map((key) => [key, new Date(responseData[key])]); |
There was a problem hiding this comment.
[correctness]
The conversion of date strings to Date objects in fetchUserProfileCompletedness may result in invalid dates if the input is not a valid date string. Consider adding validation to ensure the date strings are valid before conversion.
|
|
||
| export const confirmProfileData = async (userHandle: string) => { | ||
| const requestUrl: string = `${TC_API_HOST}/members/${userHandle}/confirmProfile`; | ||
| const request = fetch(requestUrl, {method: 'POST', headers: {...getRequestAuthHeaders()}}); |
There was a problem hiding this comment.
[❗❗ correctness]
The fetch call in confirmProfileData does not handle potential errors or non-200 HTTP responses. Consider adding error handling to manage network failures or unexpected response statuses.
| } | ||
|
|
||
| .bannerOuter { | ||
| position: relative; |
There was a problem hiding this comment.
[maintainability]
The .bannerOuter class is defined twice with different properties. This could lead to unexpected behavior. Consider consolidating these definitions to ensure clarity and maintainability.
| } = $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 banner instead of using an inline type {key: string, date: Date}. This will improve maintainability and readability, especially if the banner structure is used elsewhere.
| <div class={styles.bannerWrap}> | ||
| <div class={styles.bannerInner}> | ||
| <Banner | ||
| userHandle={user?.handle ?? ''} |
There was a problem hiding this comment.
[correctness]
Ensure that user?.handle ?? '' is the desired fallback behavior. If user.handle is critical for the Banner component, consider handling the absence of user.handle more explicitly.
| <div class={styles.nudgeWrap}> | ||
| <div class={styles.nudgeInner}> | ||
| <Toastr userhandle={user.handle} toast={toast} on:dismiss={dismissToast} /> | ||
| <Toastr userhandle={user?.handle ?? ''} toast={toast} on:dismiss={dismissToast} /> |
There was a problem hiding this comment.
[correctness]
Ensure that user?.handle ?? '' is the desired fallback behavior. If user.handle is critical for the Toastr component, consider handling the absence of user.handle more explicitly.
| return getCookieValue(COOKIE_NAME); | ||
| } | ||
|
|
||
| const isOlderThanTreshold = (date: Date | number, treshold: number): boolean => { |
There was a problem hiding this comment.
[💡 readability]
There is a typo in the function name isOlderThanTreshold. It should be isOlderThanThreshold. This could lead to confusion and should be corrected for clarity.
| .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][1] without checking if sorted is empty could lead to an error. Consider adding a check to ensure sorted has elements before accessing.
| return; | ||
| } | ||
|
|
||
| const lastUpdateOrCofirmDate = lastProfileConfirmationDate ? Math.max(lastUpdate, +lastProfileConfirmationDate) : lastUpdate; |
There was a problem hiding this comment.
[💡 readability]
There is a typo in the variable name lastUpdateOrCofirmDate. It should be lastUpdateOrConfirmDate. This could lead to confusion and should be corrected for clarity.
| } | ||
| } | ||
|
|
||
| return JSON.parse(lastSeen); |
There was a problem hiding this comment.
[correctness]
The return statement return JSON.parse(lastSeen); assumes lastSeen is always a valid JSON string. Consider adding error handling to manage potential JSON parsing 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 transition for visibility is set to 0.01ms, which is effectively instantaneous and might 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 transition for visibility is set to 0.01ms, which is effectively instantaneous and might 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.
|
|
||
| export let userHandle: string; | ||
| export let banner: {key: string; date: Date}; | ||
| let redirectUrl: string = `${PROFILE_HOST}/${userHandle}`; |
There was a problem hiding this comment.
[security]
Consider using a template literal with URL encoding for userHandle to prevent potential issues with special characters in URLs.
| } | ||
|
|
||
| async function confirm() { | ||
| await confirmProfileData(userHandle) |
There was a problem hiding this comment.
[❗❗ correctness]
Handle potential errors from confirmProfileData(userHandle) by wrapping the call in a try-catch block to ensure the application can gracefully handle any exceptions.
| return `${day}${suffix} ${months[d.getMonth()]} ${d.getFullYear()}`; | ||
| } | ||
|
|
||
| onMount(() => { |
There was a problem hiding this comment.
[correctness]
The setTimeout delay of 15ms might be too short to ensure the component is fully mounted before setting visible to true. Consider increasing the delay or using a more reliable method to ensure the component is ready.
|
|
||
| <div class={styles.spacer} /> | ||
| <a | ||
| href={redirectUrl} |
There was a problem hiding this comment.
[❗❗ security]
Ensure that redirectUrl is properly validated or sanitized to prevent potential open redirect vulnerabilities.
| background: #D4D4D4; | ||
| width: 1px; | ||
| height: 60px; | ||
| height: var(--uninav-header-height); |
There was a problem hiding this comment.
[❗❗ correctness]
The change from a fixed height to a CSS variable var(--uninav-header-height) could potentially introduce layout issues if the variable is not defined or has an unexpected value. Ensure that --uninav-header-height is defined in the relevant scope and has a valid value.
| instancesContextStore[targetId] = ctx; | ||
|
|
||
| if (navType === 'tool' || navType === 'marketing') { | ||
| await loadNudgeApp(ctx, targetEl as Element); |
There was a problem hiding this comment.
[correctness]
The await keyword is used with loadNudgeApp, which implies that this function returns a promise. Ensure that loadNudgeApp is designed to handle potential errors, as any unhandled promise rejections could lead to runtime issues.
| @@ -138,10 +141,6 @@ async function init( | |||
| if (typeof readyCallback === 'function') { | |||
| readyCallback(); | |||
| } | |||
There was a problem hiding this comment.
[correctness]
The call to loadNudgeApp has been removed from this location and moved above. Ensure that this change does not affect the order of operations required for the application logic, as it could potentially lead to unexpected behavior if the sequence is important.
| 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, such as Array<[string, Date]>, to improve readability and avoid potential confusion with tuple types.
| * @returns Boolean | ||
| */ | ||
| export declare function dismissNudgesBasedOnHost(): boolean; | ||
| export declare function dismissNudgesBasedOnHost(exceptHost?: string): boolean; |
There was a problem hiding this comment.
[❗❗ correctness]
The addition of the exceptHost parameter to dismissNudgesBasedOnHost changes its signature. Ensure that all calls to this function are updated accordingly, and verify that the optional parameter is handled correctly in the implementation to avoid potential runtime errors.
| 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 weak typing and potential runtime errors. Consider defining specific properties or using a more constrained type if possible.
| * @returns Promise<ProfileCompletednessResponse> | ||
| */ | ||
| export declare const fetchUserProfileCompletedness: (user: AuthUser, force?: boolean) => Promise<ProfileCompletednessResponse | undefined>; | ||
| export declare const confirmProfileData: (userHandle: string) => Promise<any>; |
There was a problem hiding this comment.
[maintainability]
The return type of confirmProfileData is Promise<any>, which can obscure the actual data structure being returned. Consider specifying a more precise return type to improve type safety and maintainability.
| * @param completednessData | ||
| * @returns | ||
| */ | ||
| export declare const getBanner: (completednessData: ProfileCompletionData) => any; |
There was a problem hiding this comment.
[maintainability]
The return type of getBanner is any, which can lead to potential type safety issues. Consider specifying a more precise return type to improve type safety and maintainability.
…rtal-icon PM-3695 - engagements portal icon in tool selector
…rtal-icon increase max height
2 participants
No description provided.