[Web Components] Facet selection#6220
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 enhances the web components library by adding support for facet-based filtering. It allows consumers to precisely control which data facets are displayed for specific variables using a flexible set of configuration props. The changes include updates to the component interfaces, the underlying rendering logic, and a new utility function to handle the facet resolution logic consistently across all components. Highlights
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. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request introduces facet filtering capabilities to several web components, including Bar, Highlight, Line, Map, Ranking, and Scatter charts, by adding facetId, facetIds, and facetMapping properties. It also includes a new development route and template to demonstrate these features. A bug was identified in the getFacetId utility function where an early return prevents proper fallback logic when a variable is missing from the mapping. Additionally, it was suggested to optimize performance by parsing the mapping JSON once per render cycle rather than for every variable.
…sing out of getFacetId
keyurva
left a comment
There was a problem hiding this comment.
Thanks Nick! This is very cool. Appreciate the super-fast turnaround!
|
|
||
| @bp.route('/facet-examples') | ||
| def facet_examples(): | ||
| if os.environ.get('FLASK_ENV') == 'production': |
There was a problem hiding this comment.
nit: Consider wrapping this in a util function like isProd() or isEnvironment('production'). Currently the FLASK_ENV var is accessed from multiple places and starting to keep it confined will be useful.
There was a problem hiding this comment.
Good call! There is already a similar util function for checking if we are in a testing environment. I added one in for production that matches, and used it here.
| * @param facetId Optional fallback facet ID. | ||
| * @returns The resolved facet ID or empty string. | ||
| */ | ||
| export function getFacetId( |
There was a problem hiding this comment.
Is the preference order of various facet parameters documented somewhere? If not, consider capturing it.
… gates with that function.
… provide facet ids to the web components
Description
The various web components contain props to control the data displayed, but until now did not contain the ability to indicate specific facets.
This PR implements the first round of facet-based filtering functionality: the ability to specify facets by ID.
There are three props available, depending on how you wish to apply the facet filtering:
facetIds. This takes a space-delimited list of facet IDs (matching the variable list), and applies each facet ID to the respective variable positionally.'{"Count_Person": "123", "Median_Income_Household": "456"}'). This is the recommended approach for clarity and unambiguity, as it explicitly links a facet to a specific variable regardless of their order in the props.Conflict Resolution
If a consumer provides multiple facet props simultaneously, the component resolves the facet for each variable using the following order of precedence:
facetMapping: The component first checks if the variable has an explicit mapping in this JSON object.facetIds: If not found in the mapping, it checks for a facet ID at the matching index in thefacetIdslist.facetId: If neither of the above provides a facet ID, it falls back to the globalfacetId.Although the resolution order described above is deterministic, it is recommended only to use one of the three in any one component to avoid confusion
Behavior of
facetIds(Multiple Variables)facetIdscontains exactly one ID (e.g.,facetIds="123"), that ID will be applied to all variables, effectively behaving the same asfacetId.facetIdscontains more than one ID but fewer than the number of variables, variables at indices that exceed the list length will fall back to the globalfacetId(if provided) or default resolution.Notes
Facet Ids are currently transient in that the same facet may not have the same id over time. This will change in upcoming schema changes. However, in the meantime, use of these props should take this transience into account.
Testing and Examples
There is a page available only on dev that demonstrates this functionality across the various web components.
When running the website locally, you can access this page at:
Dev Example Page