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

Tell user to reset if there are no search results #1853

Closed
wants to merge 6 commits into from

Conversation

coverbeck
Copy link
Contributor

@coverbeck coverbeck commented Oct 19, 2023

Description
Display a message if there are no search results.

Also replaced several any with types, which helped me understand the code as I was navigating it.

On the facets with no selected values being displayed (see screenshot in dockstore/dockstore#5648), I decided to leave it as is. It's complicated... If you select a non-binary facet, e.g., language, author, organization, etc., we show all the values so you can see what's not selected:

Screen Shot 2023-10-19 at 9 13 55 AM

But when you select a binary facet, e.g., has checker workflow, open data, etc., then we only display the number of matches. This may be a result of the behavior of the ES API, but I'm not sure.

Screen Shot 2023-10-19 at 9 20 28 AM
Screen Shot 2023-10-19 at 9 20 38 AM

There's a weird intersection with the 2 types of facets both selected, and after looking at hopelessly for a bit, I decided to leave it. For example, to make the language facets disappear in the case described in the bug, we'd have to not show unselected facets for languages, but that would change the behavior in general.

Review Instructions
Navigate to the "short version" described in dockstore/dockstore#5648 -- you should see no search results and you should see a message like this:

Screen Shot 2023-10-18 at 4 57 07 PM

Issue
dockstore/dockstore#5648

Security
If there are any concerns that require extra attention from the security team, highlight them here.

Please make sure that you've checked the following before submitting your pull request. Thanks!

  • Check that your code compiles by running npm run build
  • Ensure that the PR targets the correct branch. Check the milestone or fix version of the ticket.
  • If this is the first time you're submitting a PR or even if you just need a refresher, consider reviewing our style guide
  • Do not bypass Angular sanitization (bypassSecurityTrustHtml, etc.), or justify why you need to do so
  • If displaying markdown, use the markdown-wrapper component, which does extra sanitization
  • Do not use cookies, although this may change in the future
  • Run npm audit and ensure you are not introducing new vulnerabilities
  • Do due diligence on new 3rd party libraries, checking for CVEs
  • Don't allow user-uploaded images to be served from the Dockstore domain
  • If this PR is for a user-facing feature, create and link a documentation ticket for this feature (usually in the same milestone as the linked issue). Style points if you create a documentation PR directly and link that instead.
  • Check whether this PR disables tests. If it legitimately needs to disable a test, create a new ticket to re-enable it in a specific milestone.

@coverbeck coverbeck self-assigned this Oct 19, 2023
@codecov
Copy link

codecov bot commented Oct 19, 2023

Codecov Report

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

Comparison is base (99034c9) 40.53% compared to head (1a060a1) 40.52%.
Report is 3 commits behind head on develop.

Additional details and impacted files
@@             Coverage Diff             @@
##           develop    #1853      +/-   ##
===========================================
- Coverage    40.53%   40.52%   -0.02%     
===========================================
  Files          365      365              
  Lines        11283    11285       +2     
  Branches      2896     2896              
===========================================
- Hits          4574     4573       -1     
- Misses        4408     4410       +2     
- Partials      2301     2302       +1     
Files Coverage Δ
src/app/search/state/search.service.ts 21.20% <ø> (ø)
src/app/search/search.component.ts 33.86% <0.00%> (-0.28%) ⬇️
src/app/search/query-builder.service.ts 12.06% <0.00%> (ø)

... and 2 files with indirect coverage changes

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

@@ -202,8 +202,6 @@ export class SearchComponent implements OnInit, OnDestroy {
}
});

this.hits = [];
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Did this on purpose, so that code on line 325 of search.component.html doesn't flash

@svonworl
Copy link
Contributor

On the facets with no selected values being displayed (see screenshot in dockstore/dockstore#5468), I decided to leave it as is.

I can't find a screenshot of search in dockstore/dockstore#5468, is that the correct link?

@coverbeck
Copy link
Contributor Author

On the facets with no selected values being displayed (see screenshot in dockstore/dockstore#5468), I decided to leave it as is.

I can't find a screenshot of search in dockstore/dockstore#5468, is that the correct link?

Whoops, nope, updated the description.

Copy link
Contributor

@svonworl svonworl left a comment

Choose a reason for hiding this comment

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

When you go to the main search page and type a search term with no matches, two "no" boxes are displayed:

image

@coverbeck
Copy link
Contributor Author

When you go to the main search page and type a search term with no matches, two "no" boxes are displayed:

How mortifying. Nice catch.

dockstore/dockstore#5648

Also replaced several any with types, which helped me understand the
code as I was navigating it.
Use warning styling of search term suggestion as well.
@coverbeck coverbeck force-pushed the feature/5648/missingfacets branch from f159529 to 49bcbf7 Compare October 28, 2023 00:41
Copy link

sonarcloud bot commented Oct 31, 2023

SonarCloud Quality Gate failed.    Quality Gate failed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 2 Code Smells

21.2% 21.2% Coverage
0.0% 0.0% Duplication

idea Catch issues before they fail your Quality Gate with our IDE extension sonarlint SonarLint

@coverbeck
Copy link
Contributor Author

Closing this because of some odd CircleCI behavior: https://ucsc-gi.slack.com/archives/C05EZH3RVNY/p1698784510700059.

Will create a new PR.

@coverbeck coverbeck closed this Oct 31, 2023
@coverbeck coverbeck deleted the feature/5648/missingfacets branch October 31, 2023 23:07
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