Skip to content
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

Merged

Conversation

geoffreykwan
Copy link
Member

@geoffreykwan geoffreykwan commented Mar 12, 2024

Changes

  • Implemented apply tags button in the Class Schedule view
  • Tags can be applied to a unit
  • Tags can be removed from a unit

Note: You can't create a new tag at the moment.

Test

  • Test with feat(Tag): Implement add and remove project tag endpoints WISE-API#260
  • In the Class Schedule view, select some units by clicking on their checkbox
  • Click the "Apply tags" button and make sure you can add and remove a tag from a unit using the apply tags menu. You should have an "archived" tag already created if you have previously archived any units. In the future we might not want to display the "archived" tag in this menu.
  • When you apply a tag, the tag should immediately show up in the unit card
  • When you remove a tag, the tag should immediately disappear from the unit card

Note: Adding or removing the "archived" tag on a unit will not immediately move the unit to the active or archived section.

Copy link
Member

@hirokiterashima hirokiterashima left a 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/assets/wise5/services/tagService.ts Outdated Show resolved Hide resolved
src/assets/wise5/services/tagService.ts Outdated Show resolved Hide resolved
@geoffreykwan
Copy link
Member Author

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.

Copy link
Member

@hirokiterashima hirokiterashima left a 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.

@geoffreykwan geoffreykwan merged commit 65c80ee into issue-1686-implement-unit-tagging Mar 14, 2024
@geoffreykwan geoffreykwan deleted the implement-apply-tags-button branch March 14, 2024 18:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

2 participants