Topics tab refactoring #4826
Topics tab refactoring #4826
Conversation
…ne the valid categories.
server/routes/dev_place/utils.py
Outdated
| @@ -128,6 +130,7 @@ def get_place_type_with_parent_places_links(dcid: str) -> str: | |||
| return '' | |||
|
|
|||
|
|
|||
| @cache.cached(timeout=TIMEOUT, query_string=True) | |||
There was a problem hiding this comment.
I think this needs to be @cache.memoize() to include the arguments in the cache key: https://flask-caching.readthedocs.io/en/latest/#memoization
Also-you can enable caching locally for testing by setting USE_MEMCACHE = True here
There was a problem hiding this comment.
Done and tested cache locally, confirming it's very fast now!
static/js/place/dev_place_main.tsx
Outdated
| ): string => { | ||
| const isOverview = category === "Overview"; | ||
|
|
||
| let href = `/place/${place.dcid}`; |
There was a problem hiding this comment.
Using URLSearchParams might make this more clear. Something like:
const href = `/place/${place.dcid}`;
const params = new URLSearchParams();
if (!isOverview) {
params.set("category", category);
}
if (forceDevPlaces) {
params.set("force_dev_places", "true");
}
const queryString = params.toString();
if (queryString) {
return `${href}?${queryString}`;
} else {
return href;
}
There was a problem hiding this comment.
Done - definitely cleans it up a bit, thanks!
static/js/place/dev_place_main.tsx
Outdated
| })(); | ||
| }, [place]); | ||
|
|
||
| useEffect(() => { | ||
| if (placeChartsApiResponse && placeChartsApiResponse.charts) { | ||
| const categories = new Set<string>(); |
There was a problem hiding this comment.
Is this categories set used somewhere?
There was a problem hiding this comment.
Oh i was using that before but then switched to using the translatedCategories directly, so I think we're good now. Removed.
| }); | ||
| setCategories( | ||
| ["Overview"].concat( | ||
| Object.values(placeChartsApiResponse.translatedCategoryStrings) |
There was a problem hiding this comment.
I suspect this categories list should store both the english & translated values (one to use on the page text, and one to use in the URL param for the place page)
There was a problem hiding this comment.
Also- we should add "Overview": "_translated overview value_" to the translatedCategoryStrings entry in the API response.
There was a problem hiding this comment.
ah I see. Are the translation implemented yet? I tried using it with different HL params and it didnt do anything. I assume we'll need a bit more refactoring here, for example the overview topic will need a translation too, so it should come from the backend probably.
I added a todo to figure this part out.
There was a problem hiding this comment.
The translations are implemented, but i think there's some extra wiring we need to do to get the frontend to pass the language to the API.
FR example in autopush: https://autopush.datacommons.org/api/dev-place/charts/country/LVA?category=Overview&hl=fr
There was a problem hiding this comment.
ack will see what's up with those in follow up.
Refactor the Topics menu to use a shared TopicItem for each of the buttons. Only show the Topic button for categories for which we know we have data. We add a second call for the OVERVIEW chart config even when we're on a different category because we need it to identify which topics we have. France: https://screenshot.googleplex.com/X4McSQ5qax7f9w7 USA: https://screenshot.googleplex.com/BUJQV8rxhfvvT2e
Refactor the Topics menu to use a shared TopicItem for each of the buttons.
Only show the Topic button for categories for which we know we have data. We add a second call for the OVERVIEW chart config even when we're on a different category because we need it to identify which topics we have.
France: https://screenshot.googleplex.com/X4McSQ5qax7f9w7
USA: https://screenshot.googleplex.com/BUJQV8rxhfvvT2e