-
Notifications
You must be signed in to change notification settings - Fork 115
[Vis Tools] Add more examples to landing pages #5863
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
base: master
Are you sure you want to change the base?
Conversation
…to more-examples
…to more-examples
Summary of ChangesHello @juliawu, 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 significantly enhances the Vis Tools landing pages by integrating a richer set of example visualizations. The update expands the available demonstrations for both map and timeline tools, providing users with more diverse and illustrative data exploration scenarios. This improvement aims to make the visualization features more discoverable and useful for a broader range of data analysis needs. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request adds more example links to the landing pages for the visualization tools. The changes include adding new internationalization strings for the example titles and updating the configuration file with the new links for the map and timeline tools. The implementation is straightforward, but I've found a small typo in one of the new URLs that could cause it to break.
| title: intl.formatMessage( | ||
| VisToolExampleChartMessages.closeButDifferentBerkeleyAndPiedmont | ||
| ), | ||
| url: '/tools/timeline#place=geoId%2F0606000%2C%2CgeoId%2F0656938&statsVar=Median_Income_Person__Percent_Person_18OrMoreYears_WithPoorGeneralHealth__Monthly_Median_GrossRent_HousingUnit__Count_CriminalActivities_CombinedCrime&chart=%7B%22count-none%22%3A%7B%22pc%22%3Afalse%2C%22delta%22%3Afalse%7D%2C%22age-none%22%3A%7B%22pc%22%3Afalse%2C%22delta%22%3Afalse%7D%2C%22grossRent-none%22%3A%7B%22pc%22%3Afalse%2C%22delta%22%3Afalse%7D%2C%22unemploymentRate-none%22%3A%7B%22pc%22%3Afalse%2C%22delta%22%3Afalse%7D%7D', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There appears to be a typo in the URL. The place parameter contains a double comma (,,) between the geoIds, which is likely unintentional. This could cause the link to fail. It should be a single comma.
| url: '/tools/timeline#place=geoId%2F0606000%2C%2CgeoId%2F0656938&statsVar=Median_Income_Person__Percent_Person_18OrMoreYears_WithPoorGeneralHealth__Monthly_Median_GrossRent_HousingUnit__Count_CriminalActivities_CombinedCrime&chart=%7B%22count-none%22%3A%7B%22pc%22%3Afalse%2C%22delta%22%3Afalse%7D%2C%22age-none%22%3A%7B%22pc%22%3Afalse%2C%22delta%22%3Afalse%7D%2C%22grossRent-none%22%3A%7B%22pc%22%3Afalse%2C%22delta%22%3Afalse%7D%2C%22unemploymentRate-none%22%3A%7B%22pc%22%3Afalse%2C%22delta%22%3Afalse%7D%7D', | |
| url: '/tools/timeline#place=geoId%2F0606000%2CgeoId%2F0656938&statsVar=Median_Income_Person__Percent_Person_18OrMoreYears_WithPoorGeneralHealth__Monthly_Median_GrossRent_HousingUnit__Count_CriminalActivities_CombinedCrime&chart=%7B%22count-none%22%3A%7B%22pc%22%3Afalse%2C%22delta%22%3Afalse%7D%2C%22age-none%22%3A%7B%22pc%22%3Afalse%2C%22delta%22%3Afalse%7D%2C%22grossRent-none%22%3A%7B%22pc%22%3Afalse%2C%22delta%22%3Afalse%7D%2C%22unemploymentRate-none%22%3A%7B%22pc%22%3Afalse%2C%22delta%22%3Afalse%7D%7D', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good bot. Fixed.
clincoln8
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks Julia! All of these are nits except for the last question about the "extremes" charts -- why were those chosen? Maybe just add a little extra context to the title would clarify the connection.
And then lastly I noticed that also selecting "Total Population" from the extremes link would add to an existing chart instead of becoming a new chart -- what's the logic for deciding when to make it a separate one vs adding to an existing one?
before: https://screenshot.googleplex.com/65a7QNMSccgDejM
after: https://screenshot.googleplex.com/4ps76i5iF6p6qPP
| "Title of a line chart plotting the statistical variable 'poverty' for both Berkeley, USA and Piedmont, USA", | ||
| "Title of a timeline showing the differences between Berkeley, USA and Piedmont, USA", | ||
| }, | ||
| projectedTemperatureRiseInUsa: { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I noticed that this chart shows a per capita option which I don't think makes sense -- https://screenshot.googleplex.com/AjXTGCvVRkQ2Rvs
Is this something that could be addressed as a followup?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll look into it as a follow-up! There's no built-in logic for when a per capita option is shown right now, and I think figuring out what heuristics we want to use to determine when to allow per-capita needs more discussion as a team.
static/js/i18n/i18n_tool_messages.ts
Outdated
| "Title of a map plotting the statistical variable 'no schooling completed' in counties of the USA", | ||
| }, | ||
| carbonDioxideEmissionsInWorldCountries: { | ||
| id: "carbon_dioxide_emissions_in_world_countries", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
another example where having the per capita option doesn't make sense: https://screenshot.googleplex.com/Bt5VKvv9mpSz7Ug
Do we always show that option for this tool? Or is it dynamic based on the statvar?
Also, what are "installations"?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We had a URL param that triggered us plotting the locations of power plants on the map. I don't think it works anymore though..
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm, in that case, what do you guys think of just removing this example to remove confusion? @beets @miss-o-soup
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we can remove the example, along with the straggler code for it:
https://github.com/search?q=repo%3Adatacommonsorg/website%20MAP_POINTS_PLACE_TYPE&type=code
(i'd rename the dict key to RESERVED or PREVIOUSLY_USED so we don't re-use that param)
static/js/i18n/i18n_tool_messages.ts
Outdated
| }, | ||
| closeButDifferentPaloAltoAndEastPaloAlto: { | ||
| id: "close_but_different_palo_alto_and_east_palo_alto", | ||
| defaultMessage: "Close but different: Palo Alto & East Palo Alto", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same suggestion for Berkeley and piedmont title
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good suggestion, done!
| VisToolExampleChartMessages.extremesImperialCountyAndSantaClaraCounty | ||
| ), | ||
| url: '/tools/timeline#place=geoId%2F0606000%2CgeoId%2F0656938&statsVar=Count_Person_BelowPovertyLevelInThePast12Months&chart=%7B"count"%3A%7B"pc"%3Atrue%2C"denom"%3A"Count_Person"%7D%7D', | ||
| url: "/tools/timeline#place=geoId%2F06085%2CgeoId%2F06025&statsVar=Count_Death_DiseasesOfTheCirculatorySystem__Count_Death_ExternalCauses__Median_Income_Person&chart=%7B%22none-none%22%3A%7B%22pc%22%3Afalse%2C%22delta%22%3Afalse%7D%7D", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not exactly sure why we selected these statvars to display for "extremes".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was originally pulled from the old landing page. I think the original intent was to compare Imperial and Santa Clara Counties because they sit at the opposite ends of the spectrum (Rural vs Urban, Poor vs Rich, etc), but I agree that it's confusing. I've decided to remove this example altogether.
Co-authored-by: Christie Ellks <[email protected]>
Co-authored-by: Christie Ellks <[email protected]>
Adds some more links to the landing pages of the visualization tools
Pulls some examples from the old landing pages and adds them to the new landing pages. I also added more charts to the Berkeley vs. Piedmont and Palo Alto vs. East Palo Alto examples to show that our timeline tool can show multiple plots on the same page.
I tried to add more examples to the scatter tool, but many of the links don't work, so I left the current scatter tool as-is.
Before:



After:


