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
Walkthrough #120
Walkthrough #120
Conversation
[diff-counting] Significant lines: 415. |
[deployment-bot] Deployed to https://cornell-dti.github.io/course-plan/120/index.html |
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.
Looking pretty good so far! Left a couple comments about the code. Also ran into a couple bugs when testing! And I'll test again once we figure out the initial semester issue.
- Pressing the skip button doesn't actually skip the walkthrough.
- The walkthrough is activated when your profile is edited, not just the first time you log in
I am really excited for this though, it looks so good 😄
Still need to add:
|
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.
Let's try to keep the PR sizes smaller in the future. We could work out the walkthrough in steps and just have some way to gate out the incompleted features. Got some change requests here. But great job overall!
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.
This is great! Just left a few comments about some small things to resolve first before merging, and replied to a few of Han's.
Also, just wanted to mention that the actual walkthrough modals are a bit off from the designs. The font, the padding, and the button styling are all a bit off, and the step counter ('1/4', '2/4', etc) is missing from each modal. No need to worry about that until this pull request is merged in since the functionality is all there, but let's talk about when & how to resolve those 😄.
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.
Awesome! Just fix the merge conflict and then it can be merged in!
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.
LGTM!
Summary
This pull request is for the new walk through designed by Chris. It uses IntroJs to implement the tour. So far, it implements Chris' walk through that procs after onboarding is finished. If the dashboard initializes with a semester, this will work as intended. However, it does not, so the walk through stops at the requirements bar.
Test Plan
I used the site before and after onboarding to test: