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(SearchBar): Restore explore all link #7973

Conversation

joshuaeilers
Copy link
Contributor

@joshuaeilers joshuaeilers commented May 4, 2023

Changes

  • Restores the Explore all link to the search bar
  • Adds 2 new Event types, there was one missing for the search results page and one new one added for the SearchBar
  • Added a test

Screenshot:
Screenshot 2023-05-05 at 8 49 24 AM

Checklist

  • The PR conforms to DataHub's Contributing Guideline (particularly Commit Message Format)
  • Links to related issues (if applicable)
  • Tests for the changes have been added/updated (if applicable)
  • Docs related to the changes have been added/updated (if applicable). If a new feature has been added a Usage Guide has been added for the same.
  • For any breaking change/potential downtime/deprecation/big changes an entry has been made in Updating DataHub

@github-actions github-actions bot added the product PR or Issue related to the DataHub UI/UX label May 4, 2023
@@ -70,7 +71,7 @@ const QUICK_FILTER_AUTO_COMPLETE_OPTION = {
label: <EntityTypeLabel>Filter by</EntityTypeLabel>,
options: [
{
value: '',
value: 'quick-filter-unique-key',
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixes a react duplicate key warning since I added another option without a key.

Copy link
Collaborator

Choose a reason for hiding this comment

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

nice

@@ -69,6 +69,11 @@ export const SearchResultList = ({
const entityRegistry = useEntityRegistry();
const selectedEntityUrns = selectedEntities.map((entity) => entity.urn);

const onClickExploreAll = useCallback(() => {
analytics.event({ type: EventType.SearchResultsExploreAllClickEvent });
navigateToSearchUrl({ query: '*', history });
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I dropped the page: 0 from this to be consistent with the others. The navigateToSearchUrl defaults to page=1 which appears to work the same as page=0. One less thing to think about.

@@ -104,7 +109,7 @@ export const SearchResultList = ({
style={{ fontSize: 18, color: ANTD_GRAY[8] }}
description={`No results found for "${query}"`}
/>
<Button onClick={() => navigateToSearchUrl({ query: '*', page: 0, history })}>
<Button onClick={onClickExploreAll}>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Debated pulling the ExploreAll btn into its own component. There's 3 of them. All with slightly different visuals and different event types. If we consolidate the designs it would make more sense to have a reusable component.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Agree with this. Thanks for refactoring it to be a bit easier to read!

</MockedProvider>,
);
const searchInput = getByTestId('search-input');
await waitFor(() => expect(searchInput).toBeInTheDocument());
Copy link
Collaborator

Choose a reason for hiding this comment

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

thank you for adding this!

Copy link
Collaborator

@jjoyce0510 jjoyce0510 left a comment

Choose a reason for hiding this comment

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

Everything here LGTM.

Thanks for addressing this quickly!

Congrats on your first PR!!!

@jjoyce0510
Copy link
Collaborator

Minor request: Can we add a screenshot of the new view into the PR?

Copy link
Collaborator

@chriscollins3456 chriscollins3456 left a comment

Choose a reason for hiding this comment

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

Looking good to me 👍

@@ -70,7 +71,7 @@ const QUICK_FILTER_AUTO_COMPLETE_OPTION = {
label: <EntityTypeLabel>Filter by</EntityTypeLabel>,
options: [
{
value: '',
value: 'quick-filter-unique-key',
Copy link
Collaborator

Choose a reason for hiding this comment

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

nice

@jjoyce0510 jjoyce0510 merged commit 99b75e4 into datahub-project:master May 5, 2023
@joshuaeilers joshuaeilers deleted the je--search-bar-explore-all-link branch May 5, 2023 20:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
product PR or Issue related to the DataHub UI/UX
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants