feat: selecting-deselecting-mails#615
Conversation
|
@TusharBhatt1 is attempting to deploy a commit to the Zero Team on Vercel. A member of the Team first needs to authorize it. |
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThe pull request introduces a new state variable, Changes
Possibly related PRs
Suggested reviewers
Poem
🪧 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 (
|
TusharBhatt1
changed the title
|
while this is nice, what if the user wants to skip a few selected items and pick a single email a few spots below the rest? they will be forced to pick all of them. is this correct? |
Got it - makes sense ! however this PR implements what #338 asked , so can we pick this in a follow up PR as a built on top of feature , thoughts ? |
|
wanna just add to this pr? |
|
we can merge |
If this can be merged , I'll add a new issue (feature - you mentioned) and share a follow up PR in some time - I believe that's a really cool but a different feature altogether |
|
just build within this one plz |
No problem , let's get this in as well |
|
yes thats fine but this is what im try to say: if i want to select 3 emails in the mail list:
i don't want index 3 & 4 to be selected. i only want 2, 5, and 6 to be selected |
Cool , I guess giving the feature to deselect from the selected list , would be ideal here |
|
yes. we also need to make sure that the user can select individual emails across the list |
Cool , currently onClick of email (selected or not) we open the mail - is it ok to make it : onClick of selected , it deselects rather than opening ? |
|
yes thats fine |
Cool , will add these enhancements |
|
great, lmk if you need anything |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
apps/mail/components/mail/mail-list.tsx (3)
100-100: Inconsistent naming convention for state setterThe mail state uses
setEmailas the setter function but the state variable is named-const [mail, setEmail] = useMail(); +const [mail, setMail] = useMail();
488-509: Core feature implementation looks goodThe
selectHoveredAndBelowfunction correctly implements the requested feature, allowing selection of emails from the hovered one downwards. The function also handles edge cases like empty lists and already-selected items.However, consider adding a toast notification when selecting emails to provide feedback to the user:
setMail((prev) => ({ ...prev, bulkSelected: hoveredMailId ? allIdsFromHoveredAndBelow : allIds, })); +toast.success(hoveredMailId + ? t('common.mail.selectedFromHoveredDown') + : t('common.mail.selectedAll'));
489-500: Consider visual hover indicationWhile the hover state is tracked internally, there's no clear visual indication to users which email is currently hovered. Consider adding a subtle visual cue.
className={cn( 'hover:bg-offsetLight hover:bg-primary/5 group relative flex cursor-pointer flex-col items-start overflow-clip rounded-lg border border-transparent px-4 py-3 text-left text-sm transition-all hover:opacity-100', isMailSelected || (!message.unread && 'opacity-50'), (isMailSelected || isMailBulkSelected || isKeyboardFocused) && 'border-border bg-primary/5 opacity-100', isKeyboardFocused && 'ring-primary/50 ring-2', + message.id === hoveredMailId && 'border-primary/30', )}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/mail/components/mail/mail-list.tsx(11 hunks)
🔇 Additional comments (9)
apps/mail/components/mail/mail-list.tsx (9)
13-23: Import organization looks goodThe addition of the X icon import and the reorganization of React imports with proper TypeScript types is well-structured.
91-98: Good prop extension for the hover functionalityThe addition of the
setHoveredMailIdprop to the Thread component provides a clean way to communicate the hover state back to the parent component.
128-128: Well-implemented hover state managementThe hover state is properly managed by setting the current email ID on mouse enter and clearing it on mouse leave.
Also applies to: 153-154
325-346: Great addition for email deselectionThe clear selection button (X) allows users to easily deselect individual emails from their selection, enhancing usability. This is a thoughtful improvement that matches the PR discussion about allowing deselection.
347-362: Improved layout structureMoving the date display to be next to the subject line creates a more logical and balanced UI layout.
486-487: Well-implemented hover state trackingThe addition of the
hoveredMailIdstate variable properly tracks which email is currently being hovered over, which is essential for the new selection functionality.
561-562: Hotkey handlers updated correctlyAll keyboard shortcuts are correctly updated to use the new
selectHoveredAndBelowfunction, ensuring consistent behavior across different key combinations.Also applies to: 566-567, 571-572, 576-577
594-603: Enhanced selection behaviorThe updated mail selection logic allows users to add emails to their existing selection rather than replacing it, which significantly improves the multi-selection UX.
685-685: Proper prop passingThe
setHoveredMailIdfunction is correctly passed to each Thread component, establishing the necessary communication channel.
@nizzyabi we have it in 🚀 (haven't change the existing logic for selected mail and onClick - opens the mail , I feel it's really cool and useful) : #615 (comment) |
TusharBhatt1
changed the title
TusharBhatt1
changed the title
…lect-hovered-and-below-mails
…com/TusharBhatt1/Zero into feat/select-hovered-and-below-mails
|
@nizzyabi @ahmetskilinc @MrgSub can you tell why this got closed - ig the feature is not live yet |
3 participants
Implements #338
Plus @nizzyabi mentioned some enhancements (second point below) : #615 (comment)
In total , we can now :
enhancements.mp4
Type of Change
Please delete options that are not relevant.
Areas Affected
Please check all that apply:
Security Considerations
For changes involving data or authentication:
By submitting this pull request, I confirm that my contribution is made under the terms of the project's license.
Summary by CodeRabbit
Summary by CodeRabbit