-
Notifications
You must be signed in to change notification settings - Fork 4
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
Ieee 259 add description box on teams homepage #509
Ieee 259 add description box on teams homepage #509
Conversation
…b.com/ieeeuoft/hackathon-template into IEEE-272-implement-item-incident-form
…it to pass more info about hardware for incident form
… don't submit the form
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.
Great job @KarandeepLubana! This is looking close to merging, I have left some minor comments.
projectDescription: values.projectDescription, | ||
}) | ||
); | ||
} catch (error) { |
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.
you should probably throw an error snackbar in case of an error, and also make a unit test for this snackbar
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.
actually is it necessary to have this try and catch block here? Doesn't the thunk already handle the asynchronous logic?
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.
Yeah, I believe it's not necessary
variant="outlined" | ||
disabled={!isEditing} | ||
rows={4} | ||
style={{ margin: "20px 5px 0 0" }} |
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.
can you put the styling in this file in ProjectDescription.module.scss
. Reminder for scss you need to compile in order for the css to show, refer to README.md on how to do that.
type="submit" | ||
variant="contained" | ||
disabled={!isValid || isSubmitting} | ||
style={{ |
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.
put in ProjectDescription.module.scss
type="button" | ||
variant="contained" | ||
color="secondary" | ||
style={{ width: "120px" }} |
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.
put in ProjectDescription.module.scss
type="button" | ||
variant="contained" | ||
color="primary" | ||
style={{ width: "120px" }} |
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.
put in ProjectDescription.module.scss
@@ -122,6 +123,9 @@ const Dashboard = () => { | |||
{fetchOrderError && <AlertBox error={fetchOrderError} />} | |||
{/* TODO: add back in when incident reports are completed on the frontend */} | |||
{/* <BrokenTable items={itemsBroken} openReportAlert={openBrokenTable} /> */} | |||
<ProjectDescription |
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.
You could do <ProjectDescription teamCode={team_code ?? "None"} />
to make it cleaner
@KarandeepLubana This looks fantastic! I'd like to suggest a small enhancement: we should prevent teams from submitting orders on the cart page unless they have provided a project description. Perhaps we could implement an error message in the project description section, particularly if the description is too brief (consider using a predefined constant for the minimum length). You can modify Edit: Please also add the project description to Edit: You can add an alert suggesting that users need to add a project description like in DateRestrictionAlert.tsx |
hackathon_site/event/api_views.py
Outdated
@@ -283,7 +283,6 @@ class TeamDetailView( | |||
generics.GenericAPIView, | |||
): | |||
serializer_class = TeamSerializer | |||
permission_classes = [FullDjangoModelPermissions] |
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.
is this necessary to remove, i think thats why the backend checks are failing
Great work, pls copy the changes over to the makeuoft repo as well! |
Overview
Unit Tests Created
Steps to QA