-
Notifications
You must be signed in to change notification settings - Fork 0
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
update aircraft to air-based platforms, apply linting #574
Conversation
Hey @Tammo-Feldmann |
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.
@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:
admg-casei/src/utils/use-platform-list.js
Lines 13 to 22 in 1e34db2
const groupByPlatformType = (acc, item) => | |
item.searchCategory | |
? { | |
...acc, | |
[item.searchCategory]: [...(acc[item.searchCategory] || []), item], | |
} | |
: { | |
...acc, | |
["Other"]: [...(acc["Other"] || []), item], | |
} |
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 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.
Overall, this seems like a working solution however I do recommend we simplify the code by only altering the usePlatformList()
hook.
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). |
Created a backlog issue for improvement: #575 Will merge this in. |
Addresses Issue from ADMG-Project https://github.com/NASA-IMPACT/ADMG-project/issues/816