-
Notifications
You must be signed in to change notification settings - Fork 71
Feature/additional macros #3399
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
Conversation
server/events/systems.ts
Outdated
} | ||
pubsub.publish("systemsUpdate", App.systems); | ||
}; | ||
App.on("addRnDToSimulator", ({ simulatorId, className, name, cb }) => { |
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.
Any chance we could change this to be generic, where which
is an option. That way this macro includes RnD and Engineering reports.
addExtraReportToSimulator
is what I would call it.
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.
Sounds good to me! Updated!
server/typeDefs/system.ts
Outdated
} | ||
extend type Mutation { | ||
""" | ||
Macro: Systems: Add RnD to Simulator |
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.
See note above about making this generic. Also, be sure to include the word "Report" or "Damage" here.
src/components/macros/inventory.tsx
Outdated
}); | ||
console.log("baseInventorySelectionArray", baseInventorySelectionArray); | ||
const filteredBaseInventorySelectionArray = baseInventorySelectionArray.filter((s) => (s?.inventoryAdded?.length || 0) > 0); | ||
console.log("filteredBaseInventorySelectionArray", filteredBaseInventorySelectionArray); |
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.
Remove console logs please.
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.
Ugh. Embarrassing. 😄
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.
Removed
src/components/macros/inventory.tsx
Outdated
}, [apolloClient, selectedSimulatorId]); | ||
|
||
useEffect(() => { | ||
apolloClient && apolloClient.query({ |
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.
Interesting approach. Is there a reason useQuery
doesn't work here?
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.
Nope! I just forgot that useQuery
existed, and just stuck with the direct method instead of the react lifecycle version.
I did some research and switched it over to the useQuery version. Looks like that's the recommended way in this case. 👍
src/components/macros/inventory.tsx
Outdated
/> | ||
</div> | ||
<div style={{ display: "flex", flexDirection: "column", gap: "2px", width: '40px', height: "100%", alignSelf: "center" }}> | ||
<button style={{ width: '40px', height: '40px' }} onClick={() => { |
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.
Why are you using unstyled buttons here instead of the standard UI buttons that are used everywhere else?
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.
Honestly, I just looked nice and made the save button stand out. 😅 I'll do some styling with the reactstrap buttons and see if it looks better.
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.
Much improved. It'd be great if you could get rid of more of those useEffects
, though not strictly required. Feel free to merge when you're satisfied.
src/components/macros/inventory.tsx
Outdated
} | ||
`); | ||
|
||
useEffect(() => { |
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.
Gonna nitpick again. This requires two entire React render cycles
- 1 cycle for useQuery to evaluate
- 1 cycle for useEffect
Is there a specific reason you don't do this
const availableSimulators = simulatorsData?.simulators || [];
Likewise with decks
const decks = decksData?.decks || [];
(This also means replace useLazyQuery
with useQuery
and get rid of the unnecessary useEffect)
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.
I went through and removed the extra state variables. 👍
}, [decks]); | ||
|
||
// Initialize local state from args | ||
useEffect(() => { |
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.
Another improvement - it's better to initialize state inside of useState
, not with useEffect
. Is it possible to rearrange things so all of this code is called inside the initializer function for the localInventorySelections
useState?
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.
I went through and tried to get this working, but ran into some issues. For now, I'm going to merge with the improvements. If we start seeing any issues with it, I'll go back and refactor. 👍
Description
This adds two different macros that people have asked me for.
addMultipleInventory - This allows FDs to programmatically add inventory outside of the starting simulator config inventory. If you've got a mission that has custom inventory, this'll help with a lot of that. The macro does some lookups after the fact to make sure it's being added to the correct simulator/rooms, but overall is still pretty slick.
addRnDToSimulator - This allows a custom system to be added to the RnD queue. The Magellan team has asked for this to assist with their transition. This is a cheese easy macro, single field.
Related Issue
Screenshots (if appropriate):
feature on the Thorium Docs
repo. (Include the issue or pull request url below.)