NASA-IMPACT / admg-casei

ADMG Inventory
https://impact.earthdata.nasa.gov/casei/
Apache License 2.0
1 stars 0 forks source link

update aircraft to air-based platforms, apply linting #574

Closed naomatheus closed 10 months ago

naomatheus commented 10 months ago

Addresses Issue from ADMG-Project https://github.com/NASA-IMPACT/ADMG-project/issues/816

naomatheus commented 10 months ago

Hey @Tammo-Feldmann Would you want to review this? I think this was a good way to do this, but anything I am missing?

naomatheus commented 10 months ago

@naomatheus What you have seems like it works, but I think we could simplify this by having the translation of the group name occur within the usePlatformList hook itself:

https://github.com/NASA-IMPACT/admg-casei/blob/1e34db271ff2c80ad4c667b2477ef1caf23974f9/src/utils/use-platform-list.js#L13-L22

You could do something like:

const groupByPlatformType = (acc, item) => {
  const categoryTranslations = {
    Aircraft: "Air-based platforms",
  };

  const group =
    categoryTranslations[item.searchCategory] ?? item.searchCategory ?? "Other";

  (acc[group] ??= []).push(item);

  return acc;
};

This way, we reduce the number of files and functions in this project (slightly) and all future consumers of usePlatformList would get the same results as what we are rendering in the ExplorePlatforms components.

There's some side effects with this solution. After the first time the page renders, we then have more items pushed into the platform list. It gives correct results on the first render, but then on the second render the Air-based platforms are pushed to include 320 platforms (all platforms). I'm not sure how to circumvent this. I say we merge for now and flag for further review. @alukach

naomatheus commented 10 months ago

Created a backlog issue for improvement: https://github.com/NASA-IMPACT/admg-casei/issues/575

Will merge this in.