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.

Sign up for GitHub

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

Jump to bottom

fix: remove search criteria selector and auto expand results #571

Merged
merged 2 commits into from
Apr 13, 2021
Merged

fix: remove search criteria selector and auto expand results #571

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
2472106
fix: remove search criteria and auto expand results
josephaxisa Apr 9, 2021
8639366
test: search
josephaxisa Apr 13, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
46 changes: 42 additions & 4 deletions packages/api-explorer/src/components/SideNav/SideNav.spec.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,14 +24,16 @@

*/
import React from 'react'
import { ApiModel } from '@looker/sdk-codegen'
import { ApiModel, CriteriaToSet } from '@looker/sdk-codegen'
import { pick } from 'lodash'
import userEvent from '@testing-library/user-event'
import { screen } from '@testing-library/react'
import { act, screen, waitFor } from '@testing-library/react'

import { api } from '../../test-data'
import { renderWithRouter } from '../../test-utils'
import { renderWithRouter, renderWithSearchAndRouter } from '../../test-utils'
import { defaultSearchState } from '../../reducers'
import { SideNav } from './SideNav'
import { countMethods } from './searchUtils'

describe('SideNav', () => {
const testApi = ({
Expand All @@ -41,12 +43,16 @@ describe('SideNav', () => {
const allTagsPattern = /^(Auth|ApiAuth)$/
const allTypesPattern = /^(WriteDashboard|WriteQuery)$/

test('it renders all tabs', () => {
test('it renders search, methods tab and types tab', () => {
renderWithRouter(<SideNav api={testApi} specKey={'3.1'} />)
const search = screen.getByLabelText('Search')
expect(search).toHaveProperty('placeholder', 'Search')
const tabs = screen.getAllByRole('tab', {
name: /^Methods \(\d+\)|Types \(\d+\)$/,
})
expect(tabs).toHaveLength(2)
expect(tabs[0]).toHaveTextContent(`Methods (${countMethods(testApi.tags)})`)
expect(tabs[1]).toHaveTextContent('Types (2)')
})

test('Methods tab is the default active tab', () => {
Expand Down Expand Up @@ -74,3 +80,35 @@ describe('SideNav', () => {
)
})
})

describe('Search', () => {
test('it filters methods and types on input', async () => {
renderWithSearchAndRouter(<SideNav api={api} specKey="3.1" />)
const searchPattern = 'embedsso'
const input = screen.getByLabelText('Search')
jest.spyOn(api, 'search')
await act(async () => {
/** Pasting to avoid triggering search multiple times */
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't understand the comment. Perhaps a little more clarity on why. Possibly because entering characters being entered one by results in search being triggered for each character?

Something like

Using ... triggers search multiple times. Use of paste avoids this

await userEvent.paste(input, searchPattern)
await waitFor(() => {
expect(api.search).toHaveBeenCalledWith(
searchPattern,
CriteriaToSet(defaultSearchState.criteria)
)
const methods = screen.getByRole('tab', { name: 'Methods (1)' })
const types = screen.getByRole('tab', { name: 'Types (1)' })
userEvent.click(methods)
expect(
screen.getByRole('heading', {
name: api.tags.Auth.create_sso_embed_url.summary,
})
).toBeInTheDocument()

userEvent.click(types)
expect(
screen.getByRole('heading', { name: api.types.EmbedSsoParams.name })
).toBeInTheDocument()
})
})
})
})
9 changes: 6 additions & 3 deletions packages/api-explorer/src/components/SideNav/SideNav.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,6 @@ import { HEADER_REM } from '../Header'
import { SideNavTags } from './SideNavTags'
import { SideNavTypes } from './SideNavTypes'
import { useDebounce, countMethods, countTypes } from './searchUtils'
import { SearchCriteriaSelector } from './SearchCriteriaSelector'
import { SearchMessage } from './SearchMessage'

interface SideNavProps {
Expand Down Expand Up @@ -108,8 +107,8 @@ export const SideNav: FC<SideNavProps> = ({ api, specKey }) => {
return (
<nav>
<Flex alignItems="center" pl="large" pr="large" pb="large">
<SearchCriteriaSelector />
<InputSearch
aria-label="Search"
onChange={handleInputChange}
placeholder="Search"
value={pattern}
Expand All @@ -125,7 +124,11 @@ export const SideNav: FC<SideNavProps> = ({ api, specKey }) => {
</TabList>
<TabPanels {...tabs} pt="xsmall" height={`${menuH}px`} overflow="auto">
<TabPanel>
<SideNavTags tags={tags} specKey={specKey} />
<SideNavTags
tags={tags}
specKey={specKey}
defaultOpen={!!searchResults}
/>
</TabPanel>
<TabPanel>
<SideNavTypes types={types} specKey={specKey} />
Expand Down
15 changes: 10 additions & 5 deletions packages/api-explorer/src/components/SideNav/SideNavMethods.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@

*/

import React, { FC, useContext, useState } from 'react'
import React, { FC, useContext, useEffect, useState } from 'react'
import styled from 'styled-components'
import {
Accordion,
Expand All @@ -48,7 +48,7 @@ interface MethodsProps {

const SideNavMethodsLayout: FC<MethodsProps> = ({
className,
defaultOpen,
defaultOpen = false,
methods,
tag,
specKey,
Expand All @@ -59,9 +59,7 @@ const SideNavMethodsLayout: FC<MethodsProps> = ({
const match = useRouteMatch<{ methodTag: string }>(
`/:specKey/methods/:methodTag/:methodName?`
)
const [isOpen, setIsOpen] = useState(
match ? defaultOpen || match.params.methodTag === tag : defaultOpen
)
const [isOpen, setIsOpen] = useState(defaultOpen)
const history = useHistory()

const handleOpen = () => {
Expand All @@ -70,6 +68,13 @@ const SideNavMethodsLayout: FC<MethodsProps> = ({
if (_isOpen) history.push(`/${specKey}/methods/${tag}`)
}

useEffect(() => {
const status = match
Copy link
Collaborator

Choose a reason for hiding this comment

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

setIsOpen defaults to defaultOpen but you are modifying the value if the value of defaultOpen changes. Is defaultOpen a good name?

Copy link
Contributor

Choose a reason for hiding this comment

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

We can find a better name for another PR

? defaultOpen || match.params.methodTag === tag
: defaultOpen
setIsOpen(status)
}, [defaultOpen])

return (
<Accordion
defaultOpen={defaultOpen}
Expand Down