Improved newsletter messaging when access is set to nobody#27646
Conversation
Closes https://linear.app/ghost/issue/DES-1166/support-escalation-re-email-needs-verification Previously we would imply that newsletter sending was still possible when access was set to Nobody. Access = Nobody disables newsletter sending. The messaging has been updated to reflect that. The disabled state has also been improved.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
WalkthroughThe enable-newsletters component was changed so the toggle becomes non-interactive when 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Review rate limit: 7/8 reviews remaining, refill in 7 minutes and 30 seconds.Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
apps/admin-x-settings/src/components/settings/email/enable-newsletters.tsx (1)
38-40: ⚡ Quick winExtract the effective enabled-state predicate into a single constant
Line 38 and Line 59 duplicate the same condition. Pull this into one
constto keep toggle state and status rendering from drifting.Proposed refactor
const isDisabled = membersSignupAccess === 'none'; + const isNewsletterSendingEnabled = newslettersEnabled !== 'disabled' && !isDisabled; @@ <Toggle - checked={newslettersEnabled !== 'disabled' && !isDisabled} + checked={isNewsletterSendingEnabled} direction='rtl' disabled={isDisabled} onChange={handleToggleChange} /> @@ - value: (newslettersEnabled !== 'disabled' && !isDisabled) ? (<div className='w-full'> + value: isNewsletterSendingEnabled ? (<div className='w-full'>Also applies to: 59-59
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/admin-x-settings/src/components/settings/email/enable-newsletters.tsx` around lines 38 - 40, Extract the duplicated predicate newslettersEnabled !== 'disabled' && !isDisabled into a single constant (e.g., const isNewslettersEnabled = newslettersEnabled !== 'disabled' && !isDisabled) at the top of the EnableNewsletters component or function so both the Checkbox checked prop and the status rendering use this single symbol instead of repeating the expression; update references where the condition is used (checked, any status text/indicator rendering) to use isNewslettersEnabled.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@apps/admin-x-settings/src/components/settings/email/enable-newsletters.tsx`:
- Around line 38-40: Extract the duplicated predicate newslettersEnabled !==
'disabled' && !isDisabled into a single constant (e.g., const
isNewslettersEnabled = newslettersEnabled !== 'disabled' && !isDisabled) at the
top of the EnableNewsletters component or function so both the Checkbox checked
prop and the status rendering use this single symbol instead of repeating the
expression; update references where the condition is used (checked, any status
text/indicator rendering) to use isNewslettersEnabled.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: f086aa8a-725c-4e8c-89d5-577923ce9b3c
📒 Files selected for processing (1)
apps/admin-x-settings/src/components/settings/email/enable-newsletters.tsx
Closes https://linear.app/ghost/issue/DES-1166/support-escalation-re-email-needs-verification
Previously we would imply that newsletter sending was still possible when access was set to Nobody. Access = Nobody disables newsletter sending. The messaging has been updated to reflect that. The disabled state has also been improved.