History Cleanup - Airtable 125 #457
Conversation
There was a problem hiding this comment.
@thirdeyeclub I got the history cleaned up. Was able to pull out your original commit that somehow got merged into your merge commit. I squashed it with the later 2 commits you made and rebased on master.
Left a few comments here but i think there is a little more work for us to do before we continue. If it's easier for you feel free to pull this branch down and work off of this. Or just reset your branch to this sha. I can help you with that if you need just let me know.
In the mean time, we can see if anyone else has time to get some eyes on this, but there are some comments in the review and we may need to revisit some of the logic and reduce some of the wet code going into this component.
| </Select> | ||
| </FormControl> | ||
| {/* THIS PART SPAWNS BASED ON WHAT MISSION TYPE WAS SELECTED */} | ||
| {missionType === "food-box" ? ( |
There was a problem hiding this comment.
We only have one type of mission right now, right? (@utunga).
If that is the case, we can just use this as a food box mission creation and omit the check for missionType. However, the variable can probably stay for now and leave it unused.
There was a problem hiding this comment.
Yes that is correct... but fwiw MissionType is an enum in the schema.ts somewhere
| <TextField | ||
| onChange={handleItems} | ||
| name={"amount for box " + index} | ||
| type="number" | ||
| variant="outlined" | ||
| className={classes.numSelector} | ||
| value={amount} | ||
| /> |
There was a problem hiding this comment.
Will need to consult with design. Design showing this as as select with a dropdown, but the form here is a number input that lets the user enter in negative numbers.
There was a problem hiding this comment.
Didn't mention we'll want to know what the max # items an org can add with this.
There was a problem hiding this comment.
I added a min, but for right now I think a max would be hard to find unless we had some hard data on how Organizations are gonna use our application and for what amount. I suspect it will be less than 100, but I think setting a max would be jumping the gun for now.
|
Is this stale? |
|
/CC: @thirdeyeclub |
I will pull it again and check It was last time I checked |
Yes, but I had to update the routes and made a PR off this one so you wouldnt have to mege to your fork #549 |
|
@thirdeyeclub can we close this then? |
Duplicate PR of #455 with history cleaned up.