Organizer can add missions #549
Conversation
|
@thirdeyeclub Your description says this is a duplicate. Can we close? |
There was a problem hiding this comment.
@thirdeyeclub As a general comment, it would be best to create reusable components or functions when you notice yourself writing the same thing over and over
| } | ||
| } | ||
| // Functions for changing the states | ||
| const handleChangeMissionType = (event) => { |
There was a problem hiding this comment.
@thirdeyeclub Can you DRY up these state functions please? There's no need to define this many functions that basically do the same thing, but with different variables.
| }, | ||
| titleHeader: { | ||
| color: "#3739B5", | ||
| fontSize: "42px", |
There was a problem hiding this comment.
@thirdeyeclub Please refer the theme's properties instead of setting colours and sizes manually here because it will be difficult to align with the rest of the app
| margin: "10px 0", | ||
| }, | ||
| bigTextField: { | ||
| heigh: "150px", |
| }, | ||
| selectCreateButton: { | ||
| color: "#3739B5", | ||
| maxWidth: "50%", |
There was a problem hiding this comment.
@thirdeyeclub setting a max width on the button seems arbitrary, if you want to make it wider, it's better to define paddings because it will accommodate the text label in the button better, if we end up changing the copy
| className={classes.select} | ||
| value={name} | ||
| > | ||
| <MenuItem value="Fruits and Veggies">Fruits & Veggies</MenuItem> |
There was a problem hiding this comment.
@thirdeyeclub Please populate this dropdown dynamically with the different types of Foodboxes that are offered by the organization. We can't hardcode the values here.
There was a problem hiding this comment.
When I wrote this they did not exist at all yet.
| } | ||
| > | ||
| {/* THIS WILL HAVE TO HAVE A DYNAMIC INITIAL VALUE */} | ||
| <MenuItem className={classes.menuItem} value="Not yet funded"> |
There was a problem hiding this comment.
@thirdeyeclub will you be adding dynamic values here?
There was a problem hiding this comment.
They did not exist at the time 20+ days ago when I wrote it.
| </FormControl> | ||
| <Divider variant="middle" /> | ||
| <div className={classes.dualForm}> | ||
| <FormControl className={classes.form2}> |
There was a problem hiding this comment.
@thirdeyeclub Can you name the style classes with something more meaningful than "form2", eg. "recipientDetails"?
| onChange={handleRecipient} | ||
| className={classes.dualText} | ||
| value={recipient.phoneNumber} | ||
| labelid="phone" |
There was a problem hiding this comment.
@thirdeyeclub This should be labelId. See https://material-ui.com/api/select/#main-content
| InputProps={{ | ||
| startAdornment: ( | ||
| <InputAdornment position="start"> | ||
| <PhoneIcon style={{ color: "#3739B5" }} /> |
There was a problem hiding this comment.
@thirdeyeclub Can you move this inline style out into the styles object? and we should be referring to theme colours. Hardcoding styles here one-off will make it difficult to maintain.
| <div className={classes.dualForm}> | ||
| <FormControl className={classes.form2}> | ||
| <Typography className={classes.inputHeader2}>Drop Off Details </Typography> | ||
| <TextField |
There was a problem hiding this comment.
@thirdeyeclub There should be an AddressInput you can use
|
@amylo thanks so much for the very detailed feedback. I hope this is useful for you to understand why this is not yet merged @thirdeyeclub . That said I'm worried about the fact that we also have some very closely related work going on over in the 'edit mission' epic. We should probably have you and @laredotornado connect on the question of what works and/or components can be reused between the two user stories? Probably the best thing would be to let you two (and perhaps others) work out how to attack things from here. Perhaps best discussed on the channel? I just wanted to flag it as a possibility that there is a lot of reuse that can be done here between the two different stories. |
|
I mean if it wasn't sitting in PR for almost a month - with someone just looking at it now - this wouldn't be happening. We should have meeting so we can look over all the complements and she how they align with the edit mission epic. |
|
@thirdeyeclub I have to disagree. This project has been moving a mile a minute and given the fluidity of the features and changing demand, features are bound to change. Every time I pull down master and I see code has changed, I check all my open PRs and see if merged changes has affected the branches I worked off of. If they do, I always rebase on to upstream/master to bring my feature branch up to date. We should always have our features assumed to be built off of the latest code. |
|
Yayyy 🔥🔥🔥 you did it @jwu910 :-) |
Lol hope you're well! Closing up some stale PRs that have been sitting around :( |
May be a duplicate of #565