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

Added: Highlighted staged changes in BulkTagger #8676

Conversation

QuantuM410
Copy link
Contributor

Closes #8658

This PR addresses the issue outlined in #8658 by enhancing the Bulk Tagger tool to provide visual indications of subjects staged for addition or removal. The proposed solution adds highlighting to the corresponding menu options when a subject is staged.

Technical

The PR integrates the highlighting logic to the staged changes for addition or removal. The highlighting classes are applied based on the updated menu option state. I still have some queries regarding the staged changes as mentioned here

Testing

  1. Open Manage Subjects Menu
    image

  2. Add / Remove subject to view highlighted staged changes
    image

Screenshot

Before Staging changes
image

After Staging changes
image

Stakeholders

@jimchamp

@QuantuM410
Copy link
Contributor Author

@jimchamp Can you please review this PR :)

@jimchamp
Copy link
Collaborator

jimchamp commented Jan 3, 2024

If this is ready for review, press the "Ready for review" button.
Is it ready for review? I said that there should only be yellow highlighting in this comment, but I see both yellow and grey highlighting in your screenshots...

@QuantuM410
Copy link
Contributor Author

If this is ready for review, press the "Ready for review" button. Is it ready for review? I said that there should only be yellow highlighting in this comment, but I see both yellow and grey highlighting in your screenshots...

Yeh I added grey highlighting to highlight the removed changes and yellow for the addition. I can revert the highlight color for removed to yellow :) @jimchamp

@QuantuM410 QuantuM410 marked this pull request as ready for review January 4, 2024 19:04
@QuantuM410
Copy link
Contributor Author

@jimchamp I have made this PR ready for review. If you want me to revert the color of "removed" changes to yellow instead, Ill make a commit 👍

@QuantuM410
Copy link
Contributor Author

@jimchamp Please review it this PR

@codecov-commenter
Copy link

Codecov Report

Attention: 16 lines in your changes are missing coverage. Please review.

Comparison is base (71b18af) 16.66% compared to head (4f53ae6) 16.60%.
Report is 57 commits behind head on master.

Files Patch % Lines
...y/plugins/openlibrary/js/bulk-tagger/BulkTagger.js 0.00% 15 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #8676      +/-   ##
==========================================
- Coverage   16.66%   16.60%   -0.06%     
==========================================
  Files          88       88              
  Lines        4686     4702      +16     
  Branches      835      839       +4     
==========================================
  Hits          781      781              
- Misses       3389     3404      +15     
- Partials      516      517       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Collaborator

@jimchamp jimchamp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @QuantuM410, and apologies for the massive delay. There's not a need for the level of state management that these changes add --- we need only add a single class if an option is staged. If a staged option is highlighted, people need only look for the status icon to know if the tag is being added or removed.

I've added some suggested changes that should simplify things a bit.

static/css/components/tagging-menu.less Outdated Show resolved Hide resolved
@jimchamp jimchamp added the Needs: Submitter Input Waiting on input from the creator of the issue/pr [managed] label Feb 20, 2024
@QuantuM410
Copy link
Contributor Author

@jimchamp I have implemented the required changes

Before:
Screenshot from 2024-02-22 21-49-30

After:
Screenshot from 2024-02-22 21-49-43

@QuantuM410 QuantuM410 requested a review from jimchamp February 22, 2024 16:49
@jimchamp jimchamp merged commit b82e218 into internetarchive:master Feb 26, 2024
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needs: Submitter Input Waiting on input from the creator of the issue/pr [managed]
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Bulk Tagger: Highlight staged options
3 participants