fix: changed checkboxes to toggles on notifications settings page#6175
fix: changed checkboxes to toggles on notifications settings page#6175sriramveeraghanta merged 1 commit intopreviewfrom
Conversation
WalkthroughThe pull request introduces significant changes to the Changes
Assessment against linked issues
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: 1
🧹 Outside diff range and nitpick comments (3)
web/core/components/profile/notification/email-notification-form.tsx (3)
31-48: Consider enhancing error handling and user feedbackWhile the implementation is solid, consider these improvements:
- Add loading state handling to prevent multiple simultaneous updates
- Make error messages more specific by including the setting that failed to update
Here's a suggested improvement:
const handleSettingChange = async (key: keyof IUserEmailNotificationSettings, value: boolean) => { + const settingName = key.replace(/_/g, ' '); // Convert key to readable form + const [isUpdating, setIsUpdating] = useState<boolean>(false); + + if (isUpdating) return; + + setIsUpdating(true); try { await userService.updateCurrentUserEmailNotificationSettings({ [key]: value, }); setToast({ title: "Success!", type: TOAST_TYPE.SUCCESS, - message: "Email notification setting updated successfully", + message: `${settingName} setting updated successfully`, }); } catch (err) { console.error(err); setToast({ title: "Error!", type: TOAST_TYPE.ERROR, - message: "Failed to update email notification setting", + message: `Failed to update ${settingName} setting. Please try again.`, }); + } finally { + setIsUpdating(false); } };
64-64: Fix grammatical error in description textThere's a minor grammatical error in the description text.
- Notify me when issue's properties like assignees, priority, estimates or anything else changes. + Notify me when issue properties like assignees, priority, estimates or anything else change.
Line range hint
1-182: Overall implementation successfully meets PR objectivesThe changes effectively address the visibility issues mentioned in #6174 by:
- Replacing checkboxes with more visible toggle switches
- Implementing immediate updates without requiring a Save button
- Maintaining clear, descriptive labels for each setting
The code is well-structured and implements the required functionality correctly.
Consider implementing optimistic updates to improve perceived performance, updating the UI immediately while the API call happens in the background.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
web/core/components/profile/notification/email-notification-form.tsx(7 hunks)
🔇 Additional comments (1)
web/core/components/profile/notification/email-notification-form.tsx (1)
7-7: LGTM!
The imports are correctly updated to include the new ToggleSwitch component and toast utilities.
| <ToggleSwitch | ||
| value={value} | ||
| onChange={(newValue) => { | ||
| onChange(newValue); | ||
| handleSettingChange("property_change", newValue); | ||
| }} | ||
| size="sm" | ||
| /> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider extracting toggle switch implementation to reduce duplication
The toggle switch implementation is repeated across multiple settings. Consider creating a reusable component to reduce code duplication.
Here's a suggested approach:
interface NotificationToggleProps {
name: keyof IUserEmailNotificationSettings;
control: Control<IUserEmailNotificationSettings>;
onSettingChange: (key: keyof IUserEmailNotificationSettings, value: boolean) => Promise<void>;
}
const NotificationToggle: FC<NotificationToggleProps> = ({ name, control, onSettingChange }) => (
<Controller
control={control}
name={name}
render={({ field: { value, onChange } }) => (
<ToggleSwitch
value={value}
onChange={(newValue) => {
onChange(newValue);
onSettingChange(name, newValue);
}}
size="sm"
/>
)}
/>
);Then use it like this:
-<Controller
- control={control}
- name="property_change"
- render={({ field: { value, onChange } }) => (
- <ToggleSwitch
- value={value}
- onChange={(newValue) => {
- onChange(newValue);
- handleSettingChange("property_change", newValue);
- }}
- size="sm"
- />
- )}
-/>
+<NotificationToggle
+ name="property_change"
+ control={control}
+ onSettingChange={handleSettingChange}
+/>Also applies to: 96-102, 118-125, 142-149, 166-173
2 participants
Description
Updated notification settings page checkboxes to toggles for better visibility. Removed
Savebutton for better UX, now updating a setting will automatically save notification preferences.Type of Change
Screenshots and Media (if applicable)
References
Fixes #6174.
Summary by CodeRabbit
New Features
ToggleSwitchcomponent for user notification settings, replacing the previousCheckbox.Bug Fixes
Refactor