feat(tags): implement tags dropdown in Add/Edit Task dialogs #351
Conversation
|
Thank you for opening this PR! Before a maintainer takes a look, it would be really helpful if you could walk through your changes using GitHub's review tools. Please take a moment to:
More information on how to conduct a self review: This helps make the review process smoother and gives us a clearer understanding of your thought process. Once you've added your self-review, we'll continue from our side. Thank you! |
frontend/src/components/HomeComponents/Tasks/__tests__/AddTaskDialog.test.tsx
Show resolved
Hide resolved
ShivaGupta-14
left a comment
ShivaGupta-14
left a comment
There was a problem hiding this comment.
self review done! ready for review
|
Hi @its-me-abhishek! I’ve added dropdown tests in AddTaskDialog.test.tsx since the logic is identical in both dialogs, for TaskDialog.test.tsx, I only kept the remove/save tests to avoid duplication, should I also add the same dropdown interaction tests there, or is covering it once sufficient? Let me know your preference. |
its-me-abhishek
left a comment
its-me-abhishek
left a comment
There was a problem hiding this comment.
Please look into the above comments
frontend/src/components/HomeComponents/Tasks/__tests__/AddTaskDialog.test.tsx
Show resolved
Hide resolved
frontend/src/components/HomeComponents/Tasks/__tests__/AddTaskDialog.test.tsx
Outdated
Show resolved
Hide resolved
frontend/src/components/HomeComponents/Tasks/__tests__/AddTaskDialog.test.tsx
Outdated
Show resolved
Hide resolved
frontend/src/components/HomeComponents/Tasks/__tests__/AddTaskDialog.test.tsx
Outdated
Show resolved
Hide resolved
|
on it |
- Replace text input with Select dropdown for tag selection - Add "Create new tag" option in dropdown - Display selected tags as removable badges - Add uniqueTags, isCreatingNewTag, setIsCreatingNewTag props - Add data-testid to Priority, Tags, Recur SelectTriggers Tests: - Update Select component mocks for testing - Rewrite tag tests to use dropdown selection - Add tests for dropdown selection and tag removal Fixes: CCExtractor#210
ShivaGupta-14
force-pushed
the
feat/210-tag-dropdown-selection
branch
from
06dcaa3 to
d255e26
Compare
|
done with all changes, ready for review |
| return ( | ||
| <select | ||
| data-testid="project-select" | ||
| data-testid={testId} |
There was a problem hiding this comment.
why not just directly place 'data-testid' here? instead of writing and destructuring?
| }); | ||
|
|
||
| test('removes a tag when user clicks the remove button', () => { | ||
| mockProps.isOpen = true; |
There was a problem hiding this comment.
why was this changed, though?
| } | ||
| }); | ||
|
|
||
| test('should add new tag on Enter key press', () => { |
There was a problem hiding this comment.
preferably should add similar tests here as well. most of the fields tend to be flaky otherwise
| expect(result).toBeNull(); | ||
| }); | ||
|
|
||
| it('returns trimmed tag when valid and not duplicate', () => { |
There was a problem hiding this comment.
newtag needs to be something else, like new tag, or similar, in order to make this test work
There was a problem hiding this comment.
maybe use different combinations, in this test and the next one
| <div className="col-span-3 space-y-2"> | ||
| <Select | ||
| data-testid="tags-select" | ||
| value={isCreatingNewTag ? '__CREATE_NEW__' : '__SELECT__'} |
There was a problem hiding this comment.
can it be null somehow? in case of removal of all tags, what would be the case?
| onValueChange={(value) => { | ||
| if (value === '__CREATE_NEW__') { | ||
| setIsCreatingNewTag(true); | ||
| } else if (value !== '__SELECT__') { |
There was a problem hiding this comment.
since there is no default case here, seems to be skipped in logic. ideally an if-else if should be enough
| SelectItem, | ||
| SelectTrigger, | ||
| SelectValue, | ||
| } from '@/components/ui/select'; |
There was a problem hiding this comment.
moreover, i really think this could be a bad approach for our usecase. What could be better is, inside an input field, the user can either
- Write out a tag
- Select from dropdown
Both can be done at the same time By just hiding the selector, or maybe merging them similar to this.
So if a tag exists then add it from the dropdown by selecting else just type out, press enter, and the tag gets added
There was a problem hiding this comment.
@its-me-abhishek
I’ve created this multiselect filter in frontend/src/components/HomeComponents/Tasks/AddMultiSelect.tsx, it includes search, adding, removing, and creating new items, and it can be reused for other needs if needed.
If this matches what you had in mind for the selector, I’ll go ahead and implement the rest of the functionality based on it.
Screen.Recording.2026-01-09.at.9.48.43.PM.mov
There was a problem hiding this comment.
should i update the pr with this implementation?
its-me-abhishek
left a comment
its-me-abhishek
left a comment
There was a problem hiding this comment.
Suggested some changes and fixes
|
@its-me-abhishek, I understand your suggestions, but before proceeding, I’d genuinely like to get your input. I’ve also opened a PR for the same issue where I implemented a TagSelector component with search and multi-select functionality that shows the selected items. As a user, I personally prefer this approach, but I’d really appreciate your suggestion. I’m happy to proceed with whatever you decide. |
2 participants
Description
Tests:
Fixes: #210
Checklist
npx prettier --write .(for formatting)gofmt -w .(for Go backend)npm test(for JS/TS testing)Additional Notes
Video
Screen.Recording.2025-12-30.at.10.52.35.PM.mov
Screenshots