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

[Custom DC] Fix Disappearing Stat Var bug on Map and Scatter tool #4251

Merged
merged 16 commits into from
May 31, 2024

Conversation

juliawu
Copy link
Contributor

@juliawu juliawu commented May 17, 2024

This PR fixes some bugs in the map and scatter tool on Custom DC

Bug 1: Disappearing stat vars

When a user selects a place + child place type in the dropdowns, the custom variables sometimes disappear from the sidebar. For example, when using the default OECD preview data, the OECD variables disappear from the sidebar when selecting "Asia" and "Country" in the dropdowns.

This happens because the custom variables don't have enough geographic coverage to meet the 10 place minimum set in PR #2314.

This PR sets this minimum to 1 place for custom DC (so that variables with no places are still filtered out) and leaves the 10 place minimum unchanged on main DC.

Screenshot 2024-05-17 at 4 06 45 PM

Screenshot 2024-05-28 at 3 03 55 PM

Screenshot 2024-05-28 at 3 04 25 PM

Screenshot 2024-05-28 at 3 04 50 PM

Bug 2: ResizeObserver error

This PR also fixes a console error on the scatter tool encountered while debugging:
Screenshot 2024-05-17 at 4 31 53 PM

@juliawu juliawu requested review from dwnoble, keyurva and beets May 17, 2024 23:42
@@ -46,6 +46,9 @@
{% if OVERRIDE_CSS_PATH %}
<link href="{{ OVERRIDE_CSS_PATH }}" rel="stylesheet">
{% endif %}
<script>
globalThis.isCustomDC = {{ config['CUSTOM']|int }};
Copy link
Contributor

Choose a reason for hiding this comment

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

this approach will require existing custom DC users to add this line in order to see the fix. is there a way we can push the fix to them requiring them to change any code?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Instead of setting a value in base.html, switched to using a config variable in app_env/*.py files, and setting globalThis on the tools pages themselves, instead of the template which could be overridden.

NUM_ENTITIES_EXISTENCE,
samplePlaces.length
)}
numEntitiesExistence={
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you extract this logic to a function and add some comments explaining the isCustomDC vs NUM_ENTITIES_EXISTENCE check

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good idea. Extracted to a helper function and added an explaining comment.

@juliawu
Copy link
Contributor Author

juliawu commented May 28, 2024

Thanks for the review! Because Custom DC now uses the new visualization tools by default, I also fixed this bug in the new visualization tools. Client (npm) tests have been updated to reflect the new default numEntitiesExistence = 1 when the flask config variable is not set.

@juliawu juliawu requested a review from dwnoble May 28, 2024 23:21
Copy link
Contributor

@beets beets left a comment

Choose a reason for hiding this comment

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

thanks for the fix (and sorry for the delay!)

it might be easier to reason about the setting if it were the default value to filter (1 for all tools, 10 for main dc in map and scatter). then the value can be easily overriden via config.

short of that, could we update the config var name from USE_STAT_VAR_FILTERING to FILTER_LOW_GEO_COVERAGE_STAT_VAR (or a more concise version of that)

Copy link
Contributor

@beets beets left a comment

Choose a reason for hiding this comment

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

thanks for the update (and the concise flag name)

@juliawu
Copy link
Contributor Author

juliawu commented May 31, 2024

Thanks for the reviews!

For our records: took beet's advice and switched to setting the filter value in the config, and renamed the variable name to "MIN_STAT_VAR_GEO_COVERAGE"

@juliawu juliawu merged commit b4551fc into datacommonsorg:master May 31, 2024
9 checks passed
@juliawu juliawu deleted the cdc-hierarchy branch August 16, 2024 21:04
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.

3 participants