-
Notifications
You must be signed in to change notification settings - Fork 4
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(Teacher): Implement apply tags button #1687
feat(Teacher): Implement apply tags button #1687
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Functionality works as described. I agree that we shouldn't show the archived tag- this can be removed in the future before we merge to develop.
I have a few comments/questions:
- I'm wondering if the checkbox should behave like the SelectAllItemsCheckboxComponent: none, some, and all. If some of the selected units have the "archived" tag, it should show a minus sign instead of checked? Can we reuse the SelectAllItemsCheckboxComponent in the Apply tags dropdown?
- Is the class type of tag a string? If so, we can replace occurrences of "any" with "string".
- I added some code comments inline.
src/app/teacher/apply-tags-button/apply-tags-button.component.ts
Outdated
Show resolved
Hide resolved
I've created a Tag interface and used it for parameter types. I've also removed the subscribe empty functions. I'll add the minus checkbox sign in a different PR. I'll see if we should use the SelectAllItemsCheckboxComponent for that functionality. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
Before merge, can you alphabetize the fields in ApplyTagsButtonComponent?
Another thing to consider for the future is making ApplyTagsButtonComponent into a standalone component and removing the need for ApplyTagsButtonModule.
Changes
Note: You can't create a new tag at the moment.
Test
Note: Adding or removing the "archived" tag on a unit will not immediately move the unit to the active or archived section.