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

Bulk Tagger: Move "Create subject" affordance to bottom of menu options scroll container #8776

Merged
merged 4 commits into from
Feb 26, 2024

Conversation

QuantuM410
Copy link
Contributor

@QuantuM410 QuantuM410 commented Jan 30, 2024

Closes #8652

Moved the "Create subject" affordance to the bottom of the scroll container for better visibility and prevention of duplicate subjects.

Technical

  • Moved the "Create subject" affordance to the bottom of the scroll container.
  • The functionality of updating and displaying the affordance remains unchanged and doesn't get affected from the following changes in this PR.
  • Ensured that the visibility rules remain unchanged: the affordance is hidden when the subject search input is empty.

Testing

  • Search for a subject in bulk tagger component of ILE
  • The behavior of displaying create new subject affordance has been updated as per requested whilst ensuring the visibility rules

Screenshot

out2.mp4

Stakeholders

@jimchamp

@codecov-commenter
Copy link

codecov-commenter commented Jan 30, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (d376947) 16.62% compared to head (08e94dd) 16.56%.
Report is 149 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #8776      +/-   ##
==========================================
- Coverage   16.62%   16.56%   -0.06%     
==========================================
  Files          88       89       +1     
  Lines        4698     4715      +17     
  Branches      837      841       +4     
==========================================
  Hits          781      781              
- Misses       3399     3414      +15     
- Partials      518      520       +2     

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

@jimchamp jimchamp self-assigned this Feb 1, 2024
@jimchamp
Copy link
Collaborator

I was expecting the HTML string, here, to change. Did that not work?

@jimchamp jimchamp added the Needs: Submitter Input Waiting on input from the creator of the issue/pr [managed] label Feb 21, 2024
@QuantuM410
Copy link
Contributor Author

QuantuM410 commented Feb 21, 2024

I was expecting the HTML string, here, to change. Did that not work?

Yeh I thought of that and that works fine too. The only issue I had with that was with its positioning while searching for a subject when it displays content from selected-tag-subjects or whenever the searched subject is matched with less similar subjects and the number of subjects cant fill the scroll container the create-new-subject affordance seems to move just below the last displayed subject making it look weird.
Also once the user scrolls the entire container, even though the affordance is visible it will hide again if he decides to scroll back up making it a bit tedious to scroll down the entire container again. Scrolling once should be enough for him to skim over the existing subjects according to me.

Here is how it functions with the moved HTML string to this selection-container div

<div class="selection-container hidden">

out3.mp4

@jimchamp
Copy link
Collaborator

jimchamp commented Feb 21, 2024

I can't view any of the videos that you post here.

The requirement was to move the "create subject" affordance to within the div having class selection-container. The code that you've submitted doesn't do that, and makes the bulk tagger less functional:

Screenshot from 2024-02-21 09-30-46
I'm now unable to create a subject named "b"

image
...but can do so in production

I told you the exact solution that I was expecting. Please implement that.

@QuantuM410
Copy link
Contributor Author

@jimchamp Hey! I made the asked changes. It seems the screen recordings I am uploading are facing some encoding issues in firefox browser.

After Updating the HTML string :

out.mp4

Also my previous solution seems to work on my local system for all cases including typing "b" and others too
Screenshot from 2024-02-22 20-21-45

I preferred it as it allowed the affordance to display over the searched subjects once the user scrolled the entire container instead of disappearing again if user scrolls back again.

@jimchamp jimchamp merged commit 8ca25d9 into internetarchive:master Feb 26, 2024
3 checks passed
@jimchamp
Copy link
Collaborator

I preferred it as it allowed the affordance to display over the searched subjects once the user scrolled the entire container instead of disappearing again if user scrolls back again.

Whenever you have a choice between implementing code as per the requirements and implementing the code that you prefer, I recommend choosing to implement what is required.

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: Move "Create subject" affordance to bottom of menu options scroll container
3 participants