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.

Sign up for GitHub

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

Jump to bottom

[#9438] Use newly introduced Gender enum in frontend #9440

Merged
merged 10 commits into from Feb 19, 2019
Merged

[#9438] Use newly introduced Gender enum in frontend #9440

merged 10 commits into from Feb 19, 2019

Conversation

amrut-prabhu
Copy link
Contributor

@amrut-prabhu amrut-prabhu commented Feb 14, 2019 •
edited

Fixes #9438

Outline of Solution

Changed gender field from string to enum in frontend

Student profile page:
ezgif com-video-to-gif

Instructor course student details page:
image

student course details page:
2019-02-17_00h00_05

@teammates-bot
Copy link

Hi @amrut-prabhu, these parts of your pull request do not appear to follow our contributing guidelines:

  1. PR Title
    • Issue Reference (#<issue-number>) missing.

@amrut-prabhu amrut-prabhu changed the title [9438] Use newly introduced Gender enum in frontend [#9438] Use newly introduced Gender enum in frontend Feb 14, 2019
@amrut-prabhu
Copy link
Contributor Author

@xpdavid Any other changes required?

Also, not having the Gender qualifier in gender.ts results in a Reference Error as shown here and in the screenshot below
2019-02-15_02h28_15

But, adding Gender. results in a linting error ERROR: /home/travis/build/TEAMMATES/teammates/src/web/types/gender.ts[29, 18]: Qualifier is unnecessary since 'Gender' is in scope here.

How do I fix this?

@xpdavid xpdavid added the s.Ongoing The PR is being worked on by the author(s) label Feb 15, 2019
@xpdavid
Copy link
Contributor

xpdavid commented Feb 15, 2019

Please take a look at question-edit-form.component.ts to see how we can use enum in template file (e.g. *.html)

@amrut-prabhu
Replace custom function with enumToArray pipe
991995d 
Add enumToArray dependency to unit test
@amrut-prabhu
Copy link
Contributor Author

@xpdavid Ready for review

xpdavid
xpdavid reviewed Feb 16, 2019
View reviewed changes
Copy link
Contributor

@xpdavid xpdavid left a comment

Choose a reason for hiding this comment

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

Looks good. Have you tried and checked the page is working as intended?

src/web/types/gender.ts Outdated Show resolved Hide resolved
src/web/app/pages-student/student-profile-page/student-profile-page.component.ts Outdated Show resolved Hide resolved
@amrut-prabhu
Copy link
Contributor Author

@xpdavid Made necessary changes. The page seems to be working, as shown in the gif added to the PR description.

xpdavid
xpdavid reviewed Feb 17, 2019
View reviewed changes
Copy link
Contributor

@xpdavid xpdavid left a comment

Choose a reason for hiding this comment

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

Looks good. Just some nits.

src/web/app/pages-instructor/student-profile/student-profile.component.ts Outdated Show resolved Hide resolved
src/web/app/pages-student/student-course-details-page/student-course-details-page.component.ts Outdated Show resolved Hide resolved
src/web/app/pages-student/student-profile-page/student-profile-page.component.ts Outdated Show resolved Hide resolved
xpdavid
xpdavid approved these changes Feb 19, 2019
View reviewed changes
Copy link
Contributor

@xpdavid xpdavid left a comment

Choose a reason for hiding this comment

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

LGTM!

@xpdavid xpdavid added s.FinalReview The PR is ready for final review and removed s.Ongoing The PR is being worked on by the author(s) labels Feb 19, 2019
@xpdavid xpdavid added the c.Task Other non-user-facing works, e.g. refactoring, adding tests label Feb 19, 2019
wkurniawan07
wkurniawan07 approved these changes Feb 19, 2019
View reviewed changes
Copy link
Member

@wkurniawan07 wkurniawan07 left a comment

Choose a reason for hiding this comment

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

LGTM

@wkurniawan07 wkurniawan07 added s.ToMerge The PR is approved by all reviewers including final reviewer; ready for merging and removed s.FinalReview The PR is ready for final review labels Feb 19, 2019
@wkurniawan07 wkurniawan07 added this to the V7.0.0-beta.0 milestone Feb 19, 2019
@darrenwee darrenwee merged commit 4e844b6 into TEAMMATES:master Feb 19, 2019
ChooJeremy pushed a commit to ChooJeremy/teammates that referenced this pull request Feb 22, 2019
@amrut-prabhu @ChooJeremy
[TEAMMATES#9438] Use newly introduced Gender enum in frontend (TEAMMA…
dbc191d 
…TES#9440)
@amrut-prabhu amrut-prabhu deleted the 9438-front-gender-enum branch February 24, 2019 18:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@wkurniawan07 wkurniawan07 wkurniawan07 approved these changes

@xpdavid xpdavid xpdavid approved these changes

Assignees
No one assigned
Labels
c.Task Other non-user-facing works, e.g. refactoring, adding tests s.ToMerge The PR is approved by all reviewers including final reviewer; ready for merging
Projects
None yet
Milestone
V7.0.0-beta.0
Development

Successfully merging this pull request may close these issues.

Use newly introduced Gender enum in frontend
5 participants
@amrut-prabhu @teammates-bot @xpdavid @wkurniawan07 @darrenwee