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

Debounce transporter input #729

Merged
merged 5 commits into from
Jun 14, 2024

Conversation

sheckathorne
Copy link
Contributor

Description

Debounces the transporter input in the manifest form while also reducing the minimum number of characters from 5 to 2 before results begin populating. Debounce will limit the number of requests made to the server when typing-to-search while the reduction in characters will allow the user to display results earlier.

Issue ticket number and link

#727

Checklist

  • [ x ] I have added tests that prove my fix is effective or that my feature works
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • [ x ] My changes generate no new warnings

@github-actions github-actions bot added the client Related to front end workings (React/Redux) label Jun 11, 2024
@dpgraham4401 dpgraham4401 self-requested a review June 11, 2024 18:55
@dpgraham4401
Copy link
Member

Thanks @sheckathorne!

I know we already discussed this, I'll review this before the end of this week.


describe('useDebounce hook', () => {
beforeAll(() => {
vi.useFakeTimers(); // Use fake timers
Copy link
Member

Choose a reason for hiding this comment

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

This looks like it was written by AI, which is fine (if it works and isn't doing it in some backwards way), but it doesn't make sense use comments like// use fake times to explain a function call called vi.useFakeTimers(). same for the rest of the test.

Copy link
Member

Choose a reason for hiding this comment

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

I appreciate the 100% test coverage!
image

@dpgraham4401
Copy link
Member

I notice that it's still sending a request with an empty epaID when the user moves focus away from the select input, which I believe it was doing before anyways(?)

image

We can remove that call by modifying the skip option in the useSearchRcraSitesQuery

  const { data } = useSearchRcraSitesQuery(
    {
      siteType: handlerType,
      siteId: debouncedInputValue,
    },
    { skip: skip || debouncedInputValue === '' }
  );

@sheckathorne, do you have the time to modify the PR to fix this? I would hate to not clear this technical debt while we're already here. If not I can make these changes.

dpgraham4401

This comment was marked as duplicate.

Copy link
Member

@dpgraham4401 dpgraham4401 left a comment

Choose a reason for hiding this comment

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

everything looks good, just some minor edits.

@sheckathorne
Copy link
Contributor Author

Sorry I missed your comments, I have been traveling. I will make these edits today and also admit that I had some help with writing only the tests :)

Copy link
Member

@dpgraham4401 dpgraham4401 left a comment

Choose a reason for hiding this comment

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

Awesome Possum!
Appreciate the work @sheckathorne

@dpgraham4401 dpgraham4401 merged commit e57bb47 into USEPA:main Jun 14, 2024
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
client Related to front end workings (React/Redux)
Projects
No open projects
Status: Done
Development

Successfully merging this pull request may close these issues.

2 participants