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: chart click nav #97

Merged
merged 4 commits into from
Jul 5, 2023
Merged

fix: chart click nav #97

merged 4 commits into from
Jul 5, 2023

Conversation

v-rocheleau
Copy link
Contributor

Chart click navigation was failing due to a faulty query parameters check (Search.tsx checkQueryParamsEqual).
Given the parameter "age", the function would spread each letter as its own parameter ("a", "g", "e"), causing undefined === undefined which evaluates to true always.

@@ -61,6 +65,10 @@ const RoutedSearch: React.FC = () => {
const queryParam = new URLSearchParams(location.search);
const { valid, validQueryParamsObject } = validateQuery(queryParam);
if (valid) {
console.log({
Copy link
Member

Choose a reason for hiding this comment

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

change to a console.debug with some kind of info about whats being logged, or remove

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Forgot to remove, was only to debug

const qp2Keys = Object.keys(qp2);
const params = [...new Set([...qp1Keys, ...qp2Keys])];
return params.reduce((acc, v) => acc && qp1[v] === qp2[v], true);
}
Copy link
Member

Choose a reason for hiding this comment

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

return to original form to reduce diff? or is it better as not a one-liner...

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 like one-liners, but in this case I think it is easier to read/debug this way

Copy link
Member

@davidlougheed davidlougheed left a comment

Choose a reason for hiding this comment

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

lgtm

@v-rocheleau v-rocheleau merged commit d8c0b29 into main Jul 5, 2023
2 checks passed
@v-rocheleau v-rocheleau deleted the fixes/chart-click-nav branch July 5, 2023 15:51
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.

2 participants