Remove browser chart point click links#6043
Remove browser chart point click links#6043juliawu wants to merge 2 commits intodatacommonsorg:masterfrom
Conversation
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request streamlines the browser page charts by removing the click-to-redirect functionality on observation points. This change is part of a broader effort to deprecate SPARQL API endpoints and addresses the fact that the removed feature was difficult to use, slow, and not intuitive for users. The hover behavior and observation node pages remain unaffected, ensuring core functionality is preserved while improving user experience and simplifying the architecture. Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request removes the click functionality on observation points in browser charts, which depended on a deprecated SPARQL API endpoint. The changes involve removing the backend API endpoint, its tests, and all related frontend code for handling clicks, which simplifies the components. My review includes a suggestion for improving readability and a recommendation to verify potential dead code across the application for further cleanup.
I am having trouble creating individual review comments. Click here to see my feedback.
static/js/browser/observation_chart.tsx (83-86)
To improve readability and avoid repetition, you can simplify this class name construction. The string 'no-click' is present in both branches of the ternary operator.
const svgContainerClass = `no-click${this.props.series.series.length > MAX_DOTS ? " hide-dots" : ""}`;
static/js/browser/observation_chart.tsx (214-251)
With the removal of redirectToObsPage, the helper methods it called (updateErrorMessage, loadSpinner, removeSpinner) are also gone. This might leave some dead code in the component:
- The
errorMessagefield in the state is no longer updated. - The spinner elements in the JSX are no longer used.
For better maintainability, consider verifying their usage across the entire application before removing these from the state and JSX, as they might be used in other features.
References
- Before suggesting to remove a function as dead code, verify its usage across the entire application, as it might be used in other features not directly related to the current changes (e.g., disaster maps, place search).
This PR removes the click functionality of the observation points in browser page charts.
Context
We are deprecating all SPARQL API endpoints as part of the Spanner Graph migration project. This PR removes a usage of the SPAQRL API that cannot be replaced with existing
v2API calls. We use it to fetch the DCID of the observation being clicked on, so we can redirect the user to its observation browser page (e.g. https://datacommons.org/browser/dc/o/h7vbenjee1ypc).However, this click functionality itself is hard to activate (really hard to hover over the point exactly), takes a while to render (we get a spinner before redirecting the user) and it's not obvious to the user that it's a feature. Thus, we are removing this functionality altogether.
Changes Made
This PR:
Note that this PR does not:
Screenshot
On hover, we still show the point value, but the cursor stays default instead of turning into a pointer.