feat(tags): implement tags dropdown in Add/Edit Task dialogs #351
Conversation
- 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
|
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
|
|
||
| return ( | ||
| <select | ||
| data-testid="project-select" |
There was a problem hiding this comment.
i think the previous approach of using data-testid is better, and makes tests more deterministic
| const tagsInput = screen.getByPlaceholderText(/add a tag/i); | ||
| fireEvent.keyDown(tagsInput, { key: 'Enter', code: 'Enter' }); | ||
| const option = screen.queryByRole('option', { name: 'work' }); | ||
| expect(option).not.toBeInTheDocument(); |
There was a problem hiding this comment.
should we hide duplicate tags? disabling them seems to be another option, for a better UX
| render(<AddTaskdialog {...mockProps} />); | ||
|
|
||
| const workElements = screen.getAllByText('work'); | ||
| expect(workElements).toHaveLength(1); |
There was a problem hiding this comment.
this can pass even if work appears in, maybe selected tags, perhaps use getByRole with work for that
| }); | ||
|
|
||
| test('sets isCreatingNewTag to true when "create new tag" is selected', () => { | ||
| mockProps.isOpen = true; |
There was a problem hiding this comment.
can move all these mockProps to the top, and use beforeEach
| onChange={(e) => | ||
| onUpdateState({ editTagInput: e.target.value }) | ||
| } | ||
| onKeyDown={(e) => { |
There was a problem hiding this comment.
this can be moved to a helper function, inside task-utils
| e.key === 'Enter' && | ||
| editState.editTagInput.trim() | ||
| ) { | ||
| if ( |
There was a problem hiding this comment.
can use ternary operators instead of if else
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