-
Notifications
You must be signed in to change notification settings - Fork 5
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
nsc-events_sprint_12_issue_561 #562
base: main
Are you sure you want to change the base?
Conversation
…ed the recommendations from the material ui docs
Hello, |
Great job working on the TypeScript config file! I ran all the unit tests after the changes, and everything works successfully. All six test suites, including creator.test.tsx, admin.test.tsx, signin.test.tsx, and signup.test.tsx, executed without any issues, confirming that the TypeScript settings are properly integrated. The tests took around 8.5 seconds in total. Here’s a useful resource: TypeScript Compiler Options Explained. |
New and removed dependencies detected. Learn more about Socket for GitHub ↗︎
|
made some changes with version lens
update node to 20.x
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.
I'm also encountering the same issue that @miqo1385 is seeing:
I did have a conflict that I needed to resolve first which was downgrading react and react-dom to 18.2 (accepting current change) which I believe you were using (correct me if wrong). Not sure if this is the issue at hand or not. You may need to update this branch with main to see if that resolves the issue since its been a while at it says at the bottom.
Otherwise try this:
you can try to remove the import statements and simply specify the actual path in the src
attribute for the Image components used by next/image
. (have to go through each import for that in each file and correct them)
example:
<Image src="/images/google_play.png" alt="Google Play" width={100} height={100} />
I think you have to omit the public/ part because Next.js doesn't recognize the public folder as a module.
On a side note: I was also encountering some css warnings here in these images for the css files:
don't know if those were just on my end or not but thought it was worth mentioning.
not sure I should approve this just yet until these issues are addressed or clarified because it could still just be a small issue on our ends.
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.
Everything seems to work properly with these changes, would you be willing to explain what these changes are doing?
My understanding is that for rules such as "noImplicitAny": true,
, it will show an error if a type for a variable (such as a function parameter) isn't explicit in the code?
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.
Looks good overall, I tested your branch and only found a minor issue with your package-lock.json file involving syntax errors:
Im wondering if you see the same issues, if not its probably on my end then.
Regardless I think you should update this branch with main (that can probably fix the issue) and there's also an issue with the CI/CD with one of the checks not going through I believe @JesseCaddell has solved the issue so I think updating the branch with main could potentially solve the issue as well (correct me if I'm wrong), or I think you have to make another PR for the check to trigger. Once you update your branch with main ill approve this PR, thanks for the work. @Bejarano03
Please fill out this description with the following:
Resolves 561
Description of what this pull request is for.
This pull request updates the typescript config file to follow recommendations from material ui.
Screenshot: