Skip to content
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

Profile UI for CCE Minor Experiences. #1293

Open
wants to merge 17 commits into
base: development
Choose a base branch
from
Open

Conversation

seedyjahateh
Copy link
Contributor

Completed the backend for both Summer Experience and Other Engagement forms. Also implemented an 'Edt' and 'Withdraw' functionality for both forms.

Fixing issue #1152

Copy link
Contributor

@BrianRamsay BrianRamsay left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • No text in phone number field
  • Supervisor phone and email should not be required
  • Required fields should have the red * (we have that style from somewhere else)
  • Summer experience should connect through the IndividualRequirement table for the minor. See: Admin minor management page
  • No big duplicate chunks for read-only vs edit
  • Use jquery everywhere instead of vanilla javascript

app/__init__.py Outdated


if __name__ == '__main__':
app.run()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

None of this is necessary in init.py

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

resolved



@minor_bp.route('/cceMinor/<username>/viewProposal', methods=['GET'])
def viewProposal(username):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This route duplicates the already existing page.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

resolved

summerYear = CharField()
roleDescription = TextField()
experienceType = CharField()
CceMinorContentArea = TextField() # Store as comma-separated values or use a related table if needed
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This name is weird, because all summer experiences are part of the CCE Minor (also don't capitalize variables, and it should be plural). let's just go with contentAreas

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed

supervisorName = CharField()
supervisorPhone = CharField()
supervisorEmail = CharField()
created_at = DateTimeField(default=datetime.datetime.now)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

createdOn, for consistency

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed

created_at = DateTimeField(default=datetime.datetime.now)

class Meta:
db_table = 'summerExperience'
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not necessary to have this meta class, since we are creating the table (and it isn't consistent naming with our other tables)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed

<script src="https://code.jquery.com/ui/1.12.1/jquery-ui.js"></script>
<link rel="stylesheet" href="//code.jquery.com/ui/1.12.1/themes/base/jquery-ui.css">
<script src="https://code.jquery.com/jquery-3.6.0.min.js"></script>
<script src="https://code.jquery.com/ui/1.12.1/jquery-ui.js"></script>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You include jquery and jquery ui twice, even though they are already included. Also, stylesheets don't go in the scripts block (and are already included anyway).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed

other_experience_description = form_data.get('otherExperienceDescription', '')
if not other_experience_description:
raise ValueError("Other experience description is required.")
experience_type = other_experience_description
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's just assign experience_type above, and not use an additional variable.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed



@minor_bp.route('/cceMinor/<username>/updateSummerExperience', methods=['GET', 'POST'])
def updateSummerExperience(username):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Update and create need to be combined or otherwise share functionality


<div class="form-group">
<label for="experienceHoursOver300"><strong>Will your experience include at least 300 hours of work over the span of at least 8 weeks?</strong></label><br>
<input type="radio" id="yes300hours" name="experienceHoursOver300" value="Yes" {% if summer_experience.experienceHoursOver300 %}checked{% endif %} onclick="toggleTextarea()">
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have already added a listener to these inputs, so we don't need to put an onclick in the html

app/static/js/profile.js Outdated Show resolved Hide resolved
@bledsoef
Copy link
Contributor

@ojmakinde ojmakinde self-assigned this Oct 25, 2024
@ojmakinde ojmakinde removed their assignment Nov 8, 2024
@Karina-Agliullova
Copy link
Contributor

Copy link

View Code Coverage

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants