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: search in custom mode #806

Merged
merged 2 commits into from
Aug 21, 2023
Merged

fix: search in custom mode #806

merged 2 commits into from
Aug 21, 2023

Conversation

ckniffen
Copy link
Collaborator

High Level Overview of Change

Custom mode searching did not work after the react-router 6 updates.

Type of Change

  • Bug fix (non-breaking change which fixes an issue)
  • Refactor (non-breaking change that only restructures code)

Test Plan

Adding tests for custom mode proved tricky. Want to get this fix out sooner.

Custom mode searching did not work after the react-router 6 updates
path.indexOf(buildPath(VALIDATOR_ROUTE, { identifier: '' })) === 0

// NOTE: for submenus, remove `path` field and add `children` array of objects
export const navigationConfig: NavigationMenuAnyRoute[] = [
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Moved to fix circular dependencies.

@@ -0,0 +1,29 @@
import { buildPath } from '../shared/routing'
Copy link
Collaborator

Choose a reason for hiding this comment

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

There's no code change in this file, right (just moving)? I did a quick skim and it doesn't look like it, but I figured I should confirm

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is a new file. Its contents came from routes.ts to prevent circular dependencies caused by import routes into the Search component.

Copy link
Collaborator

@mvadari mvadari left a comment

Choose a reason for hiding this comment

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

please add a test

@ckniffen
Copy link
Collaborator Author

please add a test

I spent an hour trying to get tests setup. I tried switching to the MemoryRouter but since that does not update the actual brower location i couldn't come up with a good way to validate it had changed.

@mvadari
Copy link
Collaborator

mvadari commented Aug 18, 2023

please add a test

I spent an hour trying to get tests setup. I tried switching to the MemoryRouter but since that does not update the actual brower location i couldn't come up with a good way to validate it had changed.

Can you add a TODO then?

@ckniffen ckniffen requested a review from mvadari August 18, 2023 19:28
return 'ledgers'
return {
type: 'ledgers',
path: buildPath(LEDGER_ROUTE, { identifier: id }),
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this id be .toUpperCase() like the other IDs?

Copy link
Contributor

@JST5000 JST5000 left a comment

Choose a reason for hiding this comment

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

Overall LGTM, couple spots I had questions about, but I don't think they impact the change.

@ckniffen ckniffen merged commit c3849a8 into staging Aug 21, 2023
4 checks passed
@ckniffen ckniffen deleted the bugfix/search-for-custom branch August 21, 2023 23:44
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.

None yet

3 participants