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: remove search criteria selector and auto expand results #571

Merged
merged 2 commits into from
Apr 13, 2021

Conversation

josephaxisa
Copy link
Contributor

No description provided.

Copy link
Contributor

@joeldodge79 joeldodge79 left a comment

Choose a reason for hiding this comment

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

can you remove the (ApiExplorer) scope from the PR title? That should cause the pre-populated squash merge commit title to also not have it but worth checking there too when you go to do it.

I'm about to post about it in the sdk dev channel.

@josephaxisa josephaxisa changed the title fix(ApiExplorer): Removed search criteria selector and made results auto expand fix: Removed search criteria selector and made results auto expand Apr 9, 2021
jkaster
jkaster previously approved these changes Apr 9, 2021
Copy link
Contributor

@jkaster jkaster left a comment

Choose a reason for hiding this comment

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

Looks pretty simple

@jkaster
Copy link
Contributor

jkaster commented Apr 9, 2021

Something else we'll have to add tests for when/before we have codecov 😉

@josephaxisa
Copy link
Contributor Author

Something else we'll have to add tests for when/before we have codecov 😉

@jkaster Unacceptable. I added search tests :)

@josephaxisa josephaxisa changed the title fix: Removed search criteria selector and made results auto expand fix: remove search criteria selector and auto expand results Apr 13, 2021
Copy link
Collaborator

@bryans99 bryans99 left a comment

Choose a reason for hiding this comment

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

Not sure. See comments. Can discuss if you want or approve if you think its okay.

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

@@ -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

@jkaster jkaster dismissed joeldodge79’s stale review April 13, 2021 17:53

PR description updated

Copy link
Contributor

@jkaster jkaster left a comment

Choose a reason for hiding this comment

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

LGTM. Minor internal tweaks can be done later.

@jkaster jkaster merged commit e5a5ee7 into main Apr 13, 2021
@jkaster jkaster deleted the jax/search-criteria-selector branch April 13, 2021 17:56
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.

4 participants