mozilla / addons

☂ Umbrella repository for Mozilla Addons ✨
Other
127 stars 41 forks source link

Searched text is not removed from the search field while navigating through AMO #10106

Closed ValentinaPC closed 6 years ago

ValentinaPC commented 7 years ago

Steps to reproduce:

  1. Search for something in AMO-dev using your Android device
  2. After search, navigate to other pages

Expected results: Searched text is removed from search field.

Actual results: Searched text is kept in the search field.

Notes/Issues: -> if page is refreshed, the searched text is removed. Verified AMO-dev on FF51(Win 7). Video for this issue: videotogif_2017 02 08_14 58 02

aayushsanghavi commented 7 years ago

I'd like to take it up. Could you suggest how I should go about it?

kumar303 commented 7 years ago

Hi @aayushsanghavi , thanks for your interest!

@muffinresearch did we decide we definitely want to fix this? I think so but I can't remember.

The way to go about it would be to clear the search term after the user is finished searching. This won't be trivial though because it's not clear to me how to determine when the user is finished searching -- is it when a new page loads? (and how do we know when a new page loads?) or is it when they navigate away?

Ultimately, this component should switch to Redux for state management as well (so we can benefit from Redux developer tools and diagnostics) but that can come later.

mstriemer commented 7 years ago

Hopefully not adding too much noise but it might be good to take an approach similar to how the masthead will wrap the header in an <h1> if this is the homepage. So instead of clearing the state it is just hidden if the user isn't on the search page.

App tells MastHead when it's on the homepage, it could also tell it when it is searching and pass that along to SearchForm which would pass something like hideValue={!isSearching} to SearchInput. It sucks that we need to pass the props so far down but maybe that can be cleaned up.

I think SearchInput would need to ignore the hideValue prop once it receives new input. I'm guessing this would mean that it needs to store hideValue in state, set it to false in onInput and set it to true in componentWillReceiveProps if the prop changes from false to true.

Essentially, this seems like it should be fairly straightforward but for technical reason I have a feeling this will be a little tricky.

kumar303 commented 7 years ago

We also talked about adding a simple X in the search input so that users can clear it as they wish to. This would be the easiest thing to implement.

muffinresearch commented 7 years ago

Would it be possible to only populate the input when there's a search query string? I think that might cover the cases and negate the need to clear it explicitly and instead just populate it when a search is underway?

mstriemer commented 7 years ago

I think that would work for deciding the initial value of hideValue but I think you'd still need to add some extra logic like I suggested above.

I'm not totally sure but I think if you just hid the value unless you had a query string then I think you'd only be able to type into the field when you're on the search page.

muffinresearch commented 6 years ago

I don't think this is a problem we need to worry about fixing.