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

fix expandable search bar issue w/ the dropdown options (quick search) #553

Conversation

hotzjacobb
Copy link
Contributor

Description

This PR addresses issue #528. The issue was actually broader than described. The expandable search bar was actually unable to use the dropdown options after any user input keystrokes. This PR fixes that and it now works as intended.

Technical

The existing code didn't work because it tried to modify a LitElement component from outside the component. LitElement, like similar frameworks, updates the DOM based on changes to state inside components. Also, the previous implementation placed a non-documented obligation on the consumer to know that they must write this supporting code.

This implementation fixes this by changing state from within the component.

I also would like to note that I discovered another pre-existing problem while fixing this, but that goes beyond the scope of this PR. Both this.searchTerm and this.searchInput.value hold overlapping state information for text in the search bar. I tried to see if there was a simple fix by looking at LitElement's documentation, but it seemed like it would take more digging or someone with more experience with that framework. A simple solution might be converting this to React. I can file an issue if need be.

Testing

See if the expandable search bar correctly takes new input after from the drop-down menu/the expandable search bar works in general.

Evidence

iaux_expandable_search_fix_demo.mov

@codecov
Copy link

codecov bot commented Apr 18, 2021

Codecov Report

Merging #553 (f9ae562) into master (f50c11a) will increase coverage by 0.00%.
The diff coverage is 100.00%.

❗ Current head f9ae562 differs from pull request most recent head 9a85178. Consider uploading reports for the commit 9a85178 to get more accurate results
Impacted file tree graph

@@           Coverage Diff           @@
##           master     #553   +/-   ##
=======================================
  Coverage   97.64%   97.64%           
=======================================
  Files          98       98           
  Lines        1825     1828    +3     
  Branches      169      170    +1     
=======================================
+ Hits         1782     1785    +3     
  Misses         43       43           
Impacted Files Coverage Δ
packages/ia-topnav/src/ia-topnav.js 93.87% <ø> (ø)
...expandable-search-bar/src/expandable-search-bar.ts 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f50c11a...9a85178. Read the comment docs.

Copy link
Contributor

@jbuckner jbuckner left a comment

Choose a reason for hiding this comment

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

I tested the branch and the functionality looks good except for focusing the search box after clearing. Update that and add the return type to updated() and this PR should be good to go.

@hotzjacobb hotzjacobb requested a review from jbuckner June 3, 2021 01:06
Copy link
Contributor

@jbuckner jbuckner left a comment

Choose a reason for hiding this comment

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

Looks good! Thank you for the fix!

@jbuckner jbuckner merged commit 83db038 into internetarchive:master Jun 3, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants