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

Disable Click-to-Search for Non-Searchable Fields in Overview #168

Merged
merged 3 commits into from
Aug 7, 2024

Conversation

SanjeevLakhwani
Copy link
Contributor

This PR addresses the issue where clicking on non-searchable fields in the bento_public overview would trigger a search, leading to confusion as it returns results for the entire dataset.

There is now a clickable field on charts that is populated after the overview and query search fields are loaded and then there is a comparison. After this i done the charts are loaded to avoid unnecessary rerenders

@SanjeevLakhwani SanjeevLakhwani requested a review from gsfk June 20, 2024 17:25
@SanjeevLakhwani SanjeevLakhwani self-assigned this Jun 20, 2024
Copy link
Member

@gsfk gsfk left a comment

Choose a reason for hiding this comment

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

No-click working fine. I'm puzzled by all the awaits though.

src/js/components/BentoAppRouter.tsx Outdated Show resolved Hide resolved
@SanjeevLakhwani SanjeevLakhwani requested a review from gsfk July 3, 2024 15:43
Copy link
Member

@gsfk gsfk left a comment

Choose a reason for hiding this comment

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

Hmm.... the followUpPromises array actually launches all the dispatch calls rather than collecting them together. They have already run by the time you get to Promise.all.

Also, if the intention is to run these all concurrently note that getBeaconConfig() can be dispatched only after makeGetConfigRequest(), that's why there's this line in the old code:

dispatch(makeGetConfigRequest()).then(() => dispatch(getBeaconConfig()));

If you run them concurrently you'll get race conditions and it will break randomly.

@SanjeevLakhwani
Copy link
Contributor Author

Hmm.... the followUpPromises array actually launches all the dispatch calls rather than collecting them together. They have already run by the time you get to Promise.all.

Also, if the intention is to run these all concurrently note that getBeaconConfig() can be dispatched only after makeGetConfigRequest(), that's why there's this line in the old code:

dispatch(makeGetConfigRequest()).then(() => dispatch(getBeaconConfig()));

If you run them concurrently you'l get race conditions and it will break randomly.

resolved it now, it is working without issues

@SanjeevLakhwani SanjeevLakhwani requested a review from gsfk July 29, 2024 17:56
Copy link
Member

@gsfk gsfk left a comment

Choose a reason for hiding this comment

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

Working okay.

Failed intermittently for me on Chrome: sometimes produces only the arrow cursor and refuses to search anything. Happens pretty rarely so I can't give you a hint on how to reproduce it.

The clickable area on some bar charts is pretty small, although perhaps this is outside the scope of this particular pr.

@SanjeevLakhwani SanjeevLakhwani merged commit 6cba135 into main Aug 7, 2024
2 checks passed
@davidlougheed davidlougheed deleted the add-clickable-propert branch August 26, 2024 15:11
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