-
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?
| }, | ||
| noSchoolingCompletedInUsa: { | ||
| id: "no_schooling_completed_in_the_usa", | ||
| defaultMessage: "No schooling completed in the USA", |
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.
| defaultMessage: "No schooling completed in the USA", | |
| defaultMessage: "Population with no schooling completed in the USA", |
| "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"?
| defaultMessage: "Berkeley & Piedmont poverty", | ||
| closeButDifferentBerkeleyAndPiedmont: { | ||
| id: "close_but_different_berkeley_and_piedmont", | ||
| defaultMessage: "Close but different: Berkeley & Piedmont", |
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.
suggestion: add some sort of lead-in for the type of stats that will be shown when clicking this chip
| defaultMessage: "Close but different: Berkeley & Piedmont", | |
| defaultMessage: "Close but different: Quality of life indicators in Berkeley & Piedmont", |
| }, | ||
| 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
| 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".
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:


