-
Notifications
You must be signed in to change notification settings - Fork 178
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
Added typescript and update old dependencies to React v.18 #237
base: master
Are you sure you want to change the base?
Conversation
fixed re-export types
7cb2f5a
to
794c36d
Compare
Thanks alot for doing this <3 |
Any chance this could be reviewed and 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.
great
@seveibar any chance you can look at this? |
Hey guys, taking a look |
@@ -0,0 +1,7 @@ | |||
# Default ignored files |
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 remove all the .idea
files and add the .idea
directory to .gitignore
@@ -1,3 +1,3 @@ | |||
{ | |||
"semi": false | |||
"semi": true |
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.
why change this?
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 think I accidentally changed it, i will fix it
@@ -1,12 +1,15 @@ | |||
# React Image Annotate | |||
|
|||
[![npm version](https://badge.fury.io/js/react-image-annotate.svg)](https://badge.fury.io/js/react-image-annotate) | |||
[![npm version](https://img.shields.io/npm/v/@idapgroup/react-image-annotate.svg)](https://www.npmjs.com/package/@idapgroup/react-image-annotate) |
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.
we can basically remove the README changes here
"start": "react-scripts start", | ||
"test": "react-scripts test", | ||
"eject": "react-scripts eject", | ||
"storybook": "start-storybook -p 9090 -s public", |
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.
hmm removing storybook means removing a lot of tests, is there any way to keep the stories and upgrade storybook? I'm worried that we've basically lost all of our tests.
If storybook isn't your vibe, we could use react cosmos or ladle, but I think keeping the stories (which are essentially tests) will help me make sure that nothing broken.
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 don't have time for update story book and check tests(
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.
maybe in future i will update storybook too
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 generally supportive of migrating this project to Typescript, I exclusively use Typescript now, also supportive of adding people as maintainers to this project since I don't actively use it.
It's not really possible for me to test these changes because the "stories" code, has been removed. The stories are essentially tests
Would it be possible to toss this in a branch as-is and add a note in the README saying experimental React 18 support is a work in progress? |
If anyone need package with this pull, i deploy it npm.js |
I'm eager to merge this, but I think my comments are pretty reasonable, for a project this old its good to make sure you don't lose testing surface area, and some of the other changes would misdirect the user, eg the README wouldn't link to user to the npm package published by this repo. To clarify: I'm very eager to merge and make people maintainers |
No description provided.