feat(web): add an 'all' option to notification filter #964
Conversation
allows users to "reset" after selecting a filter. ideally, we'd be able to clear the filter if it was clicked again, but I couldn't find a way to listen to a second/repeat click on a SelectItem, so I added a new filter item instead.
WalkthroughThe changes involve modifications to the Changes
Possibly related PRs
Suggested reviewers
🪧 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: 0
🧹 Outside diff range and nitpick comments (2)
web/components/Notifications/List.vue (1)
Line range hint
71-74: Consider improving the empty state message.The empty state message template concatenates the importance value directly. With the new "all" filter option, when
importanceis undefined, this results in "No notifications to see here!" which is correct, but could be more explicit.Consider this improvement:
- {{ `No ${props.importance?.toLowerCase() ?? ''} notifications to see here!` }} + {{ `No ${props.importance ? props.importance.toLowerCase() + ' ' : ''}notifications to see here!` }}web/components/Notifications/Sidebar.vue (1)
92-93: Consider improving type safety for the select values.While the implementation works well, we could enhance type safety by properly typing the select value to handle both the Importance enum and the 'all' option.
Consider creating a type union:
type ImportanceFilter = Importance | 'all';Then update the Select component props and handler:
-<Select +<Select<ImportanceFilter> @update:model-value=" (val) => { - importance = val === 'all' ? undefined : (val as Importance); + importance = val === 'all' ? undefined : val; } " >
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
web/components/Notifications/List.vue(1 hunks)web/components/Notifications/Sidebar.vue(3 hunks)
🔇 Additional comments (4)
web/components/Notifications/List.vue (2)
59-61: LGTM! The loading condition is properly implemented.
The condition correctly handles both cases where there's no more data to load:
- When no items are returned (
incomingCount === 0) - When the last page is partially filled (
incomingCount < props.pageSize)
Line range hint 1-92: Verify handling of the new 'all' filter option.
The component appears to handle undefined importance prop correctly, which would correspond to the new "all" filter option. However, let's verify the integration with the parent component.
✅ Verification successful
The "all" filter option is properly integrated and handled
The verification shows that the parent component (Sidebar.vue) correctly handles the "all" filter option by setting importance to undefined when "all" is selected, which is then properly passed to the List component. The List component correctly handles this undefined value in its GraphQL query and filtering logic.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the parent component's usage of this List component
# and ensure proper handling of the new 'all' filter option
# Find parent components that import or use this List component
rg -l "NotificationsList|List\.vue" --type vue
# Check for importance prop usage in parent components
ast-grep --pattern 'importance="$_"' --lang vue
Length of output: 233
Script:
#!/bin/bash
# Search for files containing NotificationsList or List.vue
rg -l "NotificationsList|List\.vue"
# Search for importance prop usage in any file
rg "importance.*=.*" -A 2 -B 2
Length of output: 63733
web/components/Notifications/Sidebar.vue (2)
14-15: LGTM! Clean and concise implementation.
The simplified confirmation dialog maintains functionality while improving code readability.
82-82: LGTM! Clean filter reset implementation.
The implementation elegantly handles the 'all' option by setting importance to undefined, effectively resetting the filter.
|
This plugin has been deployed to Cloudflare R2 and is available for testing. |
allows users to "reset" after selecting a filter. ideally, we'd be able to
clear the filter if it was clicked again, but I couldn't find a way to listen
to a second/repeat click on a SelectItem, so I added a new filter item instead.
Summary by CodeRabbit
New Features
Improvements