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

Add graduation year #443

Merged
merged 4 commits into from
Apr 4, 2021
Merged

Add graduation year #443

merged 4 commits into from
Apr 4, 2021

Conversation

jason-tung
Copy link
Member

@jason-tung jason-tung commented Apr 4, 2021

Summary

Adds graduation field to onboarding.

https://www.notion.so/courseplan/Add-Graduation-Year-6b8a28de1ca14eb3b08b3fc8fe00cf03

Test Plan

Playtest on staging make sure nothing is broken.

Tests for Will's changes

  1. Delete your onboarding data and ensure there is no graduation year by default (so for new users). Then confirm you cannot submit onboarding without inputting it.
  2. Delete just the "gradYear" field from your onboarding (to simulate existing users) and ensure graduation year is still not selected by default.
  3. Note that only required courses have the * mark on them (major asterisks removed)
  4. Ensure the grad year dropdown autoscrolls to 2021

Will's Notes

  1. Because the onboarding modal can be clicked outside for existing users, we have no way to force users who have used our beta to input their graduation years. If this is important, we should force them to do so if the field is empty with some new modal in the future. Is this something I should make an item for to explore?
  2. The autoscroll, implemented the same as the add-semester modal, does not work as well for the onboarding modal because it is vertically larger than a screen, so it also scrolls the whole modal and not just the dropdown. If this is not visually pleasing, I could either try to change the scroll to be smooth instead of auto or remove it entirely. FIXED

Breaking Changes

Adds new 'gradYear' : string field to firestore and vuex onboarding data types.

@jason-tung jason-tung requested a review from a team as a code owner April 4, 2021 12:54
@dti-github-bot
Copy link
Member

dti-github-bot commented Apr 4, 2021

[diff-counting] Significant lines: 67.

@github-actions
Copy link
Contributor

github-actions bot commented Apr 4, 2021

Visit the preview URL for this PR (updated for commit 4bcd3d6):

https://cornelldti-courseplan-dev--pr443-add-graduation-year-cab4xod1.web.app

(expires Sun, 11 Apr 2021 15:16:56 GMT)

🔥 via Firebase Hosting GitHub Action 🌎

Copy link
Collaborator

@tcho6319 tcho6319 left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks @jason-tung and @willespencer

Because the onboarding modal can be clicked outside for existing users, we have no way to force users who have used our beta to input their graduation years. If this is important, we should force them to do so if the field is empty with some new modal in the future. Is this something I should make an item for to explore?

@willespencer yes this was a concern that I had as well. Maybe we can create a modal that appears when a user first logs in if the gradYear is undefined. One of the buttons can open up Onboarding. But yeah a Notion item to document this exploration would be great. ty

@willespencer willespencer merged commit 350b5c5 into master Apr 4, 2021
@willespencer willespencer deleted the add-graduation-year branch April 4, 2021 15:27
@willespencer willespencer mentioned this pull request Apr 21, 2021
4 tasks
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.

4 participants