tleunen / react-number-editor

Custom number editor (text field) react component
MIT License
70 stars 15 forks source link

infinite loop w/ 2.2.0 #6

Closed KyleAMathews closed 9 years ago

KyleAMathews commented 9 years ago

Haven't dug too deep. 2.1.0 works just fine.

tleunen commented 9 years ago

Hmm.. The changes between 2.1 and 2.2 is that I removed the mixin for the click and drag feature and I replaced it with the new component for it.

Do you have a specific usecase so I can easily debug it?

KyleAMathews commented 9 years ago

I do actually :) I should have added this in my initial comment actually.

Clone https://github.com/KyleAMathews/typography.js and then run npm install. Then run node_modules/.bin/webpack-dev-server then open localhost:8080. Everything should work.

Then install the latest version of react-number-editor. Then when you try dragging on the controls on the left column, the UI will freeze. If you put a console.log in the componentDidUpdate of this module, then it'll show an infinite loop.

tleunen commented 9 years ago

I couldn't make it work. I'm on Windows right now. I never used webpack before, but when I run the server, it doesn't seem to run in the right folder.

tleunen commented 9 years ago

Ok after installing webpack-dev-server globally, it's a bit better but I still can't run the project. The server is opened and serves the examples folder, and then what? I just see the list of files but nothing else.

KyleAMathews commented 9 years ago

Hrmm... I don't use Windows and don't have one that's easy to test with. What might work however is if you click the index.html file (if you see one?). If you feel up to playing with the webpack config file, the line that controls the default folder that gets served is line 11 in webpack.config.js. Perhaps the syntax there doesn't work with Windows.

cranesandcaff commented 9 years ago

Having same issue. I'll look into it as well. Is it ReactHotLoader somehow causing it?

KyleAMathews commented 9 years ago

Hmmm no I don't think so. I deployed the typography app to github (which does a production build) and saw the same problem. On Mon, May 11, 2015 at 7:51 PM Patrick Cauley notifications@github.com wrote:

Having same issue. I'll look into it as well. Is it ReactHotLoader somehow causing it?

— Reply to this email directly or view it on GitHub https://github.com/tleunen/react-number-editor/issues/6#issuecomment-101110062 .

cranesandcaff commented 9 years ago

I installed v 2.1.0 and still got it. Are you sure 2.1.0 worked?

KyleAMathews commented 9 years ago

That's what's running at http://kyleamathews.github.io/typography.js/#/

On Mon, May 11, 2015 at 7:59 PM Patrick Cauley notifications@github.com wrote:

I installed v 2.1.0 and still got it. Are you sure 2.1.0 worked?

— Reply to this email directly or view it on GitHub https://github.com/tleunen/react-number-editor/issues/6#issuecomment-101110863 .

tleunen commented 9 years ago

Ok I got the same result with a mac. The server is running but I don't have any index.html file.. Here is what I get: screen shot 2015-05-12 at 6 25 46 pm

Maybe you could create a small usecase which reproduce the issue?

KyleAMathews commented 9 years ago

Well that helped :) apparently I never checked in the index.html file. git pull and it should work now. Sorry for the bother!

tleunen commented 9 years ago

So the thing is: when you drag the value, it will change its internal state value and execute the onChange callback. When you receive the callback, your component changes its state which results in a re-render of the component (and so react-number-editor will receive new props, which results in a value change again, and here's the loop.

There is definitely something I have to fix here because it makes the entire component unusable. I'll try to take a look at this in the following days. And probably change how the component works as it is described in #5 as well.

KyleAMathews commented 9 years ago

ok, sounds great! Thanks!

cranesandcaff commented 9 years ago

I was planning on forking and fixing this today, what is the purpose of converting the number to a string?

I was planning on just moving the value to a prop and fixing the issue by containing the inputs state in it's parent component.

tleunen commented 9 years ago

Wow sorry I completely forgot about this. There is the controllable branch which should resolve most of the known issues https://github.com/tleunen/react-number-editor/tree/controllable

You can also submit PR for this branch if you see some errors in there. Let me know! I'll merge it into master in the next days.

cranesandcaff commented 9 years ago

Awesome thanks, out of curiosity though, what was/is the value of casting to the string?

KyleAMathews commented 9 years ago

Thanks! On Mon, Jul 13, 2015 at 8:18 AM Patrick Cauley notifications@github.com wrote:

Awesome thanks, out of curiosity though, what was/is the value of casting to the string?

— Reply to this email directly or view it on GitHub https://github.com/tleunen/react-number-editor/issues/6#issuecomment-120964403 .

tleunen commented 9 years ago

The string cast is to allow a custom display with a number of decimals (eg. 5.00 instead of 5). It might be possible to extract this logic from the state and put the formatting inside the render function.

I don't have a lot of time right now but can take a look to improve the behavior in the next days. I'll reuse this component for a new project so I'll probably change a few things.

cranesandcaff commented 9 years ago

Still have an infinite loop on that branch.

Why not Number.toFixed()? That should render the decimals I think...I'll check.

I'll submit a PR in a bit because I'm hoping to use this now.

I fixed the infinite loop issue, but I'm not sure if it matches the other things you wanted to change.

tleunen commented 9 years ago

Thanks for taking a look at this.

Number.toFixed is used to format the number ;)

tleunen commented 9 years ago

Releasing 3.0 now