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

utilize more types in LFNext #1664

Merged
merged 61 commits into from
Jan 10, 2023
Merged

utilize more types in LFNext #1664

merged 61 commits into from
Jan 10, 2023

Conversation

longrunningprocess
Copy link
Contributor

@longrunningprocess longrunningprocess commented Dec 24, 2022

Description

This PR begins to utilize types more extensively in the LFNext app.

Type of Change

  • Tooling update

Checklist

  • I have labeled my PR with: bug, feature, engineering, security fix or testing
  • I have performed a self-review of my own code
  • I have reviewed the title & description of this PR which I will use as the squashed PR commit message
  • I have commented my code, particularly in hard-to-understand areas
  • I have enabled auto-merge (optional)

How to test

  • exercise the change password feature
  • exercise the project dashboard feature
  • exercise the menu option while on any of the LFNext pages
  • there was a change to the way the number of entries with pictures and audio is calculated so those numbers should be checked closely to ensure their accuracy.

qa.languageforge.org testing

Testers should add his/her findings to end of the PR in a comment and include screenshots, files, etc that are beneficial.

@longrunningprocess longrunningprocess enabled auto-merge (squash) January 6, 2023 19:05
Copy link
Collaborator

@hahn-kev hahn-kev left a comment

Choose a reason for hiding this comment

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

It looks good to me. The only thing left that we discussed was throw_error, right now it only accepts a string, but in fetch we're potentially passing it an error object when there's a network error and throw_error should be updated to indicate that it accepts an Error object.

next-app/src/lib/Stats.svelte Outdated Show resolved Hide resolved
next-app/src/lib/progress/types.d.ts Outdated Show resolved Hide resolved
Copy link
Collaborator

@myieye myieye left a comment

Choose a reason for hiding this comment

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

I appreciate you putting the time into this 🤓. I mostly just have a few ideas. Feel free to use or toss whichever you wish.

next-app/src/lib/fetch.ts Show resolved Hide resolved
next-app/src/lib/server/sf.ts Show resolved Hide resolved
next-app/src/lib/server/sf.ts Show resolved Hide resolved
next-app/src/lib/progress/types.d.ts Outdated Show resolved Hide resolved
@longrunningprocess longrunningprocess merged commit f8f611f into develop Jan 10, 2023
@longrunningprocess longrunningprocess deleted the chore/types branch January 10, 2023 13:57
@myieye
Copy link
Collaborator

myieye commented Jan 12, 2023

QA 🧪

✅ exercise the change password feature
✅ exercise the project dashboard feature
✅ exercise the menu option while on any of the LFNext pages
💥 there was a change to the way the number of entries with pictures and audio is calculated so those numbers should be checked closely to ensure their accuracy.

We decided that there's nothing critical enough here to stop the release, but there is some work to be done. So, I've created #1690

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
engineering Tasks which do not directly relate to a user-facing feature or fix
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

4 participants