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

Fixes #1983: Improve SearchHelp logic #2163

Merged
merged 1 commit into from
Apr 17, 2021

Conversation

rjayroso
Copy link
Contributor

Issue This PR Addresses

Fixes #1983

Type of Change

  • Bugfix: Change which fixes an issue
  • New Feature: Change which adds functionality
  • Documentation Update: Change which improves documentation
  • UI: Change which improves UI

Description

Previously, as soon as you hit the submit button the SearchHelp would disappear. The only way to get the SearchHelp to appear again would be to refresh the page.

It would be beneficial if the SearchHelp would appear when the User's search returned no results (case A) or if the search bar was empty (case B).

This is better logically speaking since in those two additional scenarios we can assume that the User either failed their search or are about to search for something, both of which would benefit from seeing the SearchHelp. That is what this PR accomplishes.

Read more here

How to Test

  1. Search validation.js (it should return a timeline of posts)
    • scroll all the way to the end to check if the SearchHelp component truly disappeared (it should be hidden)
  2. (case A) Search fasdfsadf (it should return No Results Found)
    • the SearchHelp component should be visible
  3. (case B) Clear your search bar and hit submit
    • the SearchHelp component should be visible

Checklist

  • Quality: This PR builds and passes our npm test and works locally
  • Tests: This PR includes thorough tests or an explanation of why it does not
  • Screenshots: This PR includes screenshots or GIFs of the changes made or an explanation of why it does not (if applicable)
  • Documentation: This PR includes updated/added documentation to user exposed functionality or configuration variables are added/changed or an explanation of why it does not(if applicable)

Copy link
Contributor

@chrispinkney chrispinkney left a comment

Choose a reason for hiding this comment

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

Found a minor bug (though I think this was pointed out during our meeting today) where the search help is not shown if a you search for nothing, after searching for something.

You can see it occur in this video:
https://user-images.githubusercontent.com/7242003/115086271-edd23400-9ed9-11eb-98ff-d38fb53dcc16.mp4

Not sure if this is related to your PR or not. Looks great otherwise though!

yuanLeeMidori
yuanLeeMidori previously approved these changes Apr 16, 2021
Copy link
Contributor

@yuanLeeMidori yuanLeeMidori left a comment

Choose a reason for hiding this comment

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

Except the one Chris mentioned, all look great! This search help logic makes more sense.

@rjayroso
Copy link
Contributor Author

rjayroso commented Apr 16, 2021

@chrispinkney

Found a minor bug (though I think this was pointed out during our meeting today) where the search help is not shown if a you search for nothing, after searching for something.

You can see it occur in this video:
https://user-images.githubusercontent.com/7242003/115086271-edd23400-9ed9-11eb-98ff-d38fb53dcc16.mp4

Not sure if this is related to your PR or not. Looks great otherwise though!

  // If there is no posts or if the search bar is empty, then show the search help, otherwise hide it
  if (!error && (isEmpty || textParam.length == 0)) {
    toggleHelp(true);
  } else {
    toggleHelp(false);
  }

So I noticed in your demo that it returned a server error; how I have it right now is that if there is an error we don't display the SearchHelp, but I'm thinking - maybe this shouldn't be the case? There seems to be a weird search bug where it will return error if you search for nothing.

@chrispinkney
Copy link
Contributor

chrispinkney commented Apr 16, 2021

@chrispinkney

Found a minor bug (though I think this was pointed out during our meeting today) where the search help is not shown if a you search for nothing, after searching for something.
You can see it occur in this video:
https://user-images.githubusercontent.com/7242003/115086271-edd23400-9ed9-11eb-98ff-d38fb53dcc16.mp4
Not sure if this is related to your PR or not. Looks great otherwise though!

  // If there is no posts or if the search bar is empty, then show the search help, otherwise hide it
  if (!error && (isEmpty || textParam.length == 0)) {
    toggleHelp(true);
  } else {
    toggleHelp(false);
  }

So I noticed in your demo that it returned a server error; how I have it right now is that if there is an error we don't display the SearchHelp, but I'm thinking - maybe this shouldn't be the case? There seems to be a weird search bug where it will return error if you search for nothing.

Weird. If the bug isn't coming from your PR then yeah I think the current behaviour is fine and it should not show the search help. Could you file an issue for it?

PS Let me know when you rebase it and I can approve it 😄

@rjayroso
Copy link
Contributor Author

@yuanLeeMidori @chrispinkney rebased

@rjayroso rjayroso merged commit f765b25 into Seneca-CDOT:master Apr 17, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Improve SearchHelp logic
3 participants