[WEB-2431] chore: profile settings page UI improvement #5838
Conversation
WalkthroughThe changes in this pull request involve updates to several components and interfaces across the codebase. Key modifications include the addition of a Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 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: 6
🧹 Outside diff range and nitpick comments (5)
packages/ui/src/dropdowns/custom-search-select.tsx (2)
138-140: LGTM! Consider adding a comment for the z-index change.The changes to the class names look good. Removing
px-2from the outer div and addingz-20to increase the z-index are valid improvements. These modifications will likely enhance the layout and visibility of the dropdown.Consider adding a brief comment explaining the reason for the
z-20class addition, as it might not be immediately obvious to other developers why a specific z-index value was chosen.
Line range hint
145-152: LGTM! Consider adding an aria-label for accessibility.The addition of a styled container for the search input, including the search icon, is a great improvement to the component's visual appeal and usability. The
mx-2class ensures proper spacing within the dropdown.To enhance accessibility, consider adding an
aria-labelto the search input. For example:<Combobox.Input className="w-full bg-transparent py-1 text-xs text-custom-text-200 placeholder:text-custom-text-400 focus:outline-none" value={query} onChange={(e) => setQuery(e.target.value)} placeholder="Search" displayValue={(assigned: any) => assigned?.name} + aria-label="Search options" />web/core/components/project/form.tsx (1)
384-408: Do you need assistance with implementing the timezone selection feature?The code for the timezone selection (lines 384-408) is currently commented out, indicating that this feature might be pending implementation or deferred. If you require help in integrating the timezone functionality into the form, I can assist by providing code suggestions or handling the implementation.
Would you like me to help implement the timezone selection feature or open a new GitHub issue to track this task?
web/app/profile/page.tsx (2)
300-303: Add missing 'rounded-md' class to 'display_name' input for consistent stylingThe
rounded-mdclass is missing in theclassNameof thedisplay_nameinput field, leading to inconsistent styling compared to other input fields.Apply this diff to include the class:
- className={`w-full ${errors?.display_name ? "border-red-500" : ""}`} + className={`w-full rounded-md ${errors?.display_name ? "border-red-500" : ""}`}
Line range hint
140-153: Clarify the usage of 'url' parameter in 'handleDelete' functionThe
handleDeletefunction accepts aurlparameter but doesn't use it within the function body. This can lead to confusion about the function's intent.If
urlis unnecessary, consider removing it:- const handleDelete = async (url: string | null | undefined) => { + const handleDelete = async () => { if (!currentUser?.avatar_url) return; await updateCurrentUser({ avatar_url: "", }) // ...Alternatively, if you intend to generalize the function, use the
urlparameter:await updateCurrentUser({ avatar_url: url ? "" : undefined, })
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
- packages/types/src/project/projects.d.ts (1 hunks)
- packages/ui/src/dropdowns/custom-search-select.tsx (2 hunks)
- web/app/profile/page.tsx (4 hunks)
- web/core/components/project/form.tsx (9 hunks)
🧰 Additional context used
🔇 Additional comments (3)
packages/ui/src/dropdowns/custom-search-select.tsx (1)
Line range hint
156-161: Great improvements to the scrollable options list!The addition of
vertical-scrollbarandscrollbar-xsclasses enhances the appearance and usability of the scrollable options list. Thespace-y-1class improves readability by adding consistent vertical spacing between options. These changes will contribute to a better user experience when interacting with the dropdown.web/app/profile/page.tsx (2)
168-172: Verify calling 'handleSubmit(onSubmit)' within 'onSuccess' handlerCalling
handleSubmit(onSubmit)()inside theonSuccesshandler may lead to unexpected behavior, such as multiple form submissions or state inconsistencies. Ensure that this approach aligns with React Hook Form's best practices.Consider reviewing the documentation or testing to confirm this doesn't introduce any issues.
378-392: Confirm 'getTimeZoneLabel' integration with 'CustomSearchSelect'The
getTimeZoneLabelfunction is used to render the content of each timezone option. Verify that it correctly displays the timezone information within theCustomSearchSelectcomponent and handles all possibletimezonevalues, including undefined or null cases.
| // handlers | ||
| const handleIdentifierChange = (event: React.ChangeEvent<HTMLInputElement>) => { | ||
| const { value } = event.target; | ||
| const alphanumericValue = value.replace(/[^a-zA-Z0-9]/g, ""); | ||
| const formattedValue = alphanumericValue.toUpperCase(); | ||
| setValue("identifier", formattedValue); |
There was a problem hiding this comment.
Fix inconsistency between Project ID input handling and validation
The handleIdentifierChange function currently strips out all characters except a-z, A-Z, and 0-9, removing non-Latin characters like Ç, Ş, Ğ, İ, Ö, Ü. However, your validation rules for the identifier field allow these characters, as specified by the regex /^[ÇŞĞIİÖÜA-Z0-9]+$/. This inconsistency can lead to confusion and prevent users from inputting valid characters.
To ensure consistency between the input handling and validation, update the regex in handleIdentifierChange to include the non-Latin characters allowed by the validation.
Apply this diff to fix the inconsistency:
- const alphanumericValue = value.replace(/[^a-zA-Z0-9]/g, "");
+ const alphanumericValue = value.replace(/[^ÇŞĞIİÖÜa-zA-Z0-9]/gi, "");📝 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.
| // handlers | |
| const handleIdentifierChange = (event: React.ChangeEvent<HTMLInputElement>) => { | |
| const { value } = event.target; | |
| const alphanumericValue = value.replace(/[^a-zA-Z0-9]/g, ""); | |
| const formattedValue = alphanumericValue.toUpperCase(); | |
| setValue("identifier", formattedValue); | |
| // handlers | |
| const handleIdentifierChange = (event: React.ChangeEvent<HTMLInputElement>) => { | |
| const { value } = event.target; | |
| const alphanumericValue = value.replace(/[^ÇŞĞIİÖÜa-zA-Z0-9]/gi, ""); | |
| const formattedValue = alphanumericValue.toUpperCase(); | |
| setValue("identifier", formattedValue); |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (4)
web/app/profile/page.tsx (4)
219-220: LGTM: Improved form layout with comprehensive fieldsThe new grid layout enhances the organization of the form, making it more visually appealing and easier to navigate. The additional fields provide a more comprehensive user profile.
Consider adding some vertical spacing between the two grid sections for better visual separation:
- <div className="flex flex-col gap-2 pt-4"> + <div className="flex flex-col gap-2 pt-8">This will create a clearer distinction between the two sets of form fields.
Also applies to: 365-368
396-409: Enhance disabled state for language selectionThe "Coming soon" tooltip for the language selection is a good way to indicate future functionality. To make the disabled state more visually apparent, consider adjusting the styling:
<CustomSearchSelect value="English (US)" label="English (US)" options={[]} onChange={() => {}} - className="rounded-md bg-custom-background-90" + className="rounded-md bg-custom-background-90 opacity-60 cursor-not-allowed" input disabled />This change will make it clearer to users that the language selection is currently unavailable.
Line range hint
419-449: Enhance warning for account deactivationThe use of the
Disclosurecomponent for the "Deactivate account" section is a good choice, as it helps prevent accidental actions. To make the warning more emphatic, consider updating the text:<span className="text-sm tracking-tight"> - When deactivating an account, all of the data and resources within that account will be - permanently removed and cannot be recovered. + WARNING: Deactivating your account is irreversible. All data and resources associated with this account will be permanently deleted and cannot be recovered. Please ensure you want to proceed with this action. </span>This change emphasizes the permanence and seriousness of the action, helping users make an informed decision.
Line range hint
41-453: Consider memoization for performance optimizationThe profile settings page has been significantly improved with new functionality and better layout. To potentially enhance performance, consider memoizing some of the more complex rendered elements or computations. For example:
const MemoizedCustomSearchSelect = React.memo(CustomSearchSelect);Then use
MemoizedCustomSearchSelectinstead ofCustomSearchSelectin your render method. This could help prevent unnecessary re-renders, especially for the timezone selection component which has complex options.Additionally, you might want to memoize the
timeZoneOptionsarray:const memoizedTimeZoneOptions = React.useMemo(() => TIME_ZONES.map((timeZone) => ({ value: timeZone.value, query: timeZone.name + " " + timeZone.gmtOffset + " " + timeZone.value, content: getTimeZoneLabel(timeZone), })), [] // Empty dependency array as TIME_ZONES is likely constant );These optimizations could help improve the overall performance of the component, especially if it's rendered frequently or as part of a larger, more complex page.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- web/app/profile/page.tsx (4 hunks)
🧰 Additional context used
🔇 Additional comments (2)
web/app/profile/page.tsx (2)
9-18: LGTM: Import changes look goodThe new imports, including
Tooltipfrom@plane/uiand the addition ofTTimezoneto theTIME_ZONESimport, align well with the new functionality introduced in this update.Also applies to: 25-25
119-127: LGTM: Well-implemented timezone label formatterThe
getTimeZoneLabelfunction provides a consistent way to format timezone information for display. It handles undefined input gracefully and returns a nicely formatted label combining GMT offset and timezone name.
| <Controller | ||
| name="user_timezone" | ||
| control={control} | ||
| rules={{ required: "Please select a timezone" }} | ||
| render={({ field: { value, onChange } }) => ( | ||
| <CustomSearchSelect | ||
| value={value} | ||
| label={ | ||
| value | ||
| ? (getTimeZoneLabel(TIME_ZONES.find((t) => t.value === value)) ?? value) | ||
| : "Select a timezone" | ||
| } | ||
| options={timeZoneOptions} | ||
| onChange={onChange} | ||
| buttonClassName={errors.user_timezone ? "border-red-500" : ""} | ||
| className="rounded-md border-[0.5px] !border-custom-border-200" | ||
| optionsClassName="w-72" | ||
| input | ||
| /> | ||
| )} | ||
| /> | ||
| {errors.user_timezone && <span className="text-xs text-red-500">{errors.user_timezone.message}</span>} |
There was a problem hiding this comment.
Fix error handling for timezone field
The timezone selection implementation looks good overall, providing a user-friendly way to select timezones. However, there's a potential issue with the error handling:
- buttonClassName={errors.user_timezone ? "border-red-500" : ""}
+ buttonClassName={errors.user_timezone ? "border-red-500" : "border-custom-border-200"}This change ensures that the button always has a border, improving consistency in the UI.
📝 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.
| <Controller | |
| name="user_timezone" | |
| control={control} | |
| rules={{ required: "Please select a timezone" }} | |
| render={({ field: { value, onChange } }) => ( | |
| <CustomSearchSelect | |
| value={value} | |
| label={ | |
| value | |
| ? (getTimeZoneLabel(TIME_ZONES.find((t) => t.value === value)) ?? value) | |
| : "Select a timezone" | |
| } | |
| options={timeZoneOptions} | |
| onChange={onChange} | |
| buttonClassName={errors.user_timezone ? "border-red-500" : ""} | |
| className="rounded-md border-[0.5px] !border-custom-border-200" | |
| optionsClassName="w-72" | |
| input | |
| /> | |
| )} | |
| /> | |
| {errors.user_timezone && <span className="text-xs text-red-500">{errors.user_timezone.message}</span>} | |
| <Controller | |
| name="user_timezone" | |
| control={control} | |
| rules={{ required: "Please select a timezone" }} | |
| render={({ field: { value, onChange } }) => ( | |
| <CustomSearchSelect | |
| value={value} | |
| label={ | |
| value | |
| ? (getTimeZoneLabel(TIME_ZONES.find((t) => t.value === value)) ?? value) | |
| : "Select a timezone" | |
| } | |
| options={timeZoneOptions} | |
| onChange={onChange} | |
| buttonClassName={errors.user_timezone ? "border-red-500" : "border-custom-border-200"} | |
| className="rounded-md border-[0.5px] !border-custom-border-200" | |
| optionsClassName="w-72" | |
| input | |
| /> | |
| )} | |
| /> | |
| {errors.user_timezone && <span className="text-xs text-red-500">{errors.user_timezone.message}</span>} |
Before
After
Issue link: WEB-2431
Summary by CodeRabbit