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

Convert project to Typescript #394

Closed
wants to merge 4 commits into from
Closed

Convert project to Typescript #394

wants to merge 4 commits into from

Conversation

rhysyngsun
Copy link
Collaborator

@rhysyngsun rhysyngsun commented Jan 31, 2022 •
edited
Loading

Pre-Flight checklist

  • Testing
    • Code is tested
    • Changes have been manually tested

What are the relevant tickets?

Closes #24
Closes #40

What's this PR do?

This PR dominoed into a set of dependent changes, hence the large diff:

  • Initial conversion to typescript
    • Generally a lot of cleanup to be done as flow was not as strict despite being inflexible a lot of the time
  • Refactored HOC components to function based to make the typing easier
    • Refactored several things into hooks to make the ergonomics better, particularly the notifications system.
  • Upgraded webpack to support typescript
  • Upgraded code formatters and linters to support typescript
  • Switch to stylelint instead of sass-lint because newest prettier was causing the latter to fail.

How should this be manually tested?

Go through the app and make sure everything still works, generally:

  • Able to register a new account
  • Able to login to an existing account
  • Able to update email/password
  • Able to update profile
  • Able to enroll in a course
  • Able to interact with the dashboard options per-course

@codecov-commenter
Copy link

codecov-commenter commented Feb 3, 2022 •
edited
Loading

Codecov Report

Merging #394 (7cb7095) into main (10d87c5) will decrease coverage by 4.08%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #394      +/-   ##
==========================================
- Coverage   91.37%   87.28%   -4.09%     
==========================================
  Files         230      103     -127     
  Lines        7049     4011    -3038     
  Branches      616      616              
==========================================
- Hits         6441     3501    -2940     
+ Misses        516      416     -100     
- Partials       92       94       +2
Impacted Files Coverage Δ
static/js/components/AnonymousMenu.test.tsx
static/js/components/AnonymousMenu.tsx
static/js/components/Expandable.test.tsx
static/js/components/Expandable.tsx
static/js/components/Header.tsx
static/js/components/InstituteLogo.test.tsx
static/js/components/InstituteLogo.tsx
static/js/components/Loader.test.tsx
static/js/components/Loader.tsx
static/js/components/MixedLink.test.tsx
... and 51 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 10d87c5...7cb7095. Read the comment docs.

@rhysyngsun rhysyngsun force-pushed the nl/convert-to-ts branch 3 times, most recently from 3290166 to 5a1678e Compare February 4, 2022 14:56
@rhysyngsun rhysyngsun changed the title Nl/convert to ts Convert project to Typescript Feb 4, 2022
@rhysyngsun rhysyngsun marked this pull request as ready for review February 7, 2022 02:18
@jkachel jkachel self-requested a review February 8, 2022 13:16
@jkachel
Copy link
Contributor

jkachel commented Feb 8, 2022

These things all appeared to be OK:

  • Sign in as an existing user
  • Register as a new user (also tested with incomplete data on the post-verification screen; validation messages worked OK)
  • Change email
  • Change password
  • Update profile

Need to test course interactions still but did want to report on things that have tested OK so far.

@jkachel
Copy link
Contributor

jkachel commented Feb 9, 2022

On the dashboard, the dropdown to manage course options does not appear to work. The button styling is off (from the current RC build) and the dropdown itself doesn't activate. See screenshot:
image
Not seeing anything in the console other than a warning about using componentWillMount.

jkachel
jkachel requested changes Feb 9, 2022
View reviewed changes
Copy link
Contributor

@jkachel jkachel left a comment

Choose a reason for hiding this comment

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

The manage course dropdown seems broken (see #394 (comment)). Everything else tested OK for me.

@odlbot odlbot temporarily deployed to mitxonline-ci-pr-394 February 9, 2022 19:40 Inactive
@rhysyngsun
Copy link
Collaborator Author

Closing because this project has been refactored substantially and this branch is inoperable now.

@rhysyngsun rhysyngsun closed this Jul 20, 2022
@rhysyngsun rhysyngsun deleted the nl/convert-to-ts branch July 20, 2022 20:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jkachel jkachel jkachel requested changes

Assignees

@rhysyngsun rhysyngsun

@jkachel jkachel

Labels
Waiting on Author
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Upgrade react-redux and refactor class components to functional style Convert to Typescript
4 participants
@rhysyngsun @codecov-commenter @jkachel @odlbot