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

added one click results display directly into the preview item, now eliminating 2 unnecessary clicks #335

Merged
merged 2 commits into from Oct 22, 2020
Merged

added one click results display directly into the preview item, now eliminating 2 unnecessary clicks #335

merged 2 commits into from Oct 22, 2020

Conversation

sourabhbagrecha
Copy link
Contributor

@sourabhbagrecha sourabhbagrecha commented Oct 20, 2020

Related Issue

Fixes #62

Checklist:

  • I have read the contributor's guide.
  • I linked an issue in the previous section
  • I have commented on the linked issue
  • I was assigned the linked issue (not required)
  • I have tested the change to the best of my ability against the sandbox or a local build.

Optional items:

  • My change adds new text and requires a change to translations.
  • My change requires a change to the documentation.
  • I have submitted a PR to the documentation repo.
  • I was not able to test... (explain below, e.g. you did not have permissions to test a specific feature)
  • This change depends O-FISH Realm repository changes (explain below)
  • Optional: Add any explanations here

  • Optional: Add any relevant screenshots here

Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

Thanks for submitting your first PR to this repository. We appreciate your contribution! Our team will review your code within 3 business days. We <3 open source and are so glad you do, too!

autoEscape={true}
textToHighlight={name}
/>
<Link to={searchResultsLink}>
Copy link
Contributor

@lenmorld lenmorld Oct 21, 2020

Choose a reason for hiding this comment

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

I like how Link is a more declarative solution,
but since it wraps it in an <a>, doesn't it add some default styling to the item, like underline and active/visited colors?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, it does add some default styles.

Copy link
Contributor

Choose a reason for hiding this comment

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

To keep the component styles wrapped by Link,
we can maybe try something like this:
#327 (comment)

@sourabhbagrecha
Copy link
Contributor Author

@Sheeri Are there any blockers for this PR, lmk if anything.

@sourabhbagrecha
Copy link
Contributor Author

@Sheeri I have made the style changes, please review this commit

Copy link
Collaborator

@Sheeri Sheeri left a comment

Choose a reason for hiding this comment

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

The style changes look great!

Can the information be exposed in the URL (as a json string for now)? It does pull up the right vessel record, crew member record, etc but it would be nice if it was in the URL so it could be copy/pasteable.

@sourabhbagrecha
Copy link
Contributor Author

Yeah. I agree.
But for the current behavior of the app, this is the best we can do(if you go through the current app flow there's no json string in the url while showing the results), if I am not wrong @ayushjainrksh has started working on query params already as discussed in the last Office Hours. Since that's a completely different issue and to maintain the separation of concerns, it's better if we scope that out in a different ticket.

Ps: I would love to work on that(query string feature) if no one else is working.

@ayushjainrksh
Copy link
Contributor

Hi @sourabhbagrecha, I haven't started working on the issue yet. Please feel free to open up an issue and start working if assigned. I'll be happy to co-author PR with you if needed.

@Sheeri
Copy link
Collaborator

Sheeri commented Oct 22, 2020

OK. so I'm going to merge this, as the links can be clicked. @sourabhbagrecha you are free to work on another issue, you can go ahead and make the query params feature issue and start working on it. (I'm leaving it to you to make the issue, because you can describe the reasons to change from JSON strings better than I can.

@Sheeri Sheeri merged commit c121c6d into WildAid:main Oct 22, 2020
@Sheeri
Copy link
Collaborator

Sheeri commented Oct 22, 2020

Congratulations! 🎉 🙌 🎆 ㊗️

This PR is great and has been merged into the code. Thanks so much for contributing! If you'd like, you can request to work on another issue. We really appreciate your effort!

To claim your O-FISH badge, head on over to the MongoDB Community forums, login, and reply - making sure to link to this PR.

@sourabhbagrecha sourabhbagrecha deleted the sourabh/search_item_fix branch November 10, 2020 05:27
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.

Results in search autocomplete should go to the appropriate page
4 participants