search

webscopeio / react-textarea-autocomplete

📝 React component implements configurable GitHub's like textarea autocomplete.
MIT License
451 stars 80 forks source link

The autocomplete still won't appear in the correct spot. #124

Closed jukben closed 5 years ago

jukben commented 5 years ago

@jukben I noticed that my CodePen had started working so I was quite excited, but on upgrading to 4.2.2 in my project the autocomplete still won't appear in the correct spot. :(

I went back to the CodePen and updated it until I could repeat the problem: large gif 696x614

Here's the example URL again: https://codepen.io/andy-pearson/full/vvRLRP

Now the number of items appears randomly, there seems to be a race condition where the autocomplete doesn't quite know its own size before doing the positioning calculations.

Interestingly, this example also crashes Firefox fairly consistently with the following error:

RTA: dataProvider fails: Maximum update depth exceeded. This can happen when a component repeatedly calls setState inside componentWillUpdate or componentDidUpdate. React limits the number of nested updates to prevent infinite loops.

I think the trick would be to change _calculatePosition so that it sets the required styles directly using the DOM APIs, rather than getting React to do it with setState. Shall I try and have a go at making that change? Feel like I'm asking a lot without actually contributing!

Originally posted by @andypearson in https://github.com/webscopeio/react-textarea-autocomplete/issues/37#issuecomment-455567585

jukben commented 5 years ago

@andypearson I would start here, there could be the loop https://github.com/webscopeio/react-textarea-autocomplete/blob/master/src/Textarea.jsx#L143

I think the trick would be to change _calculatePosition so that it sets the required styles directly using the DOM APIs, rather than getting React to do it with setState. Shall I try and have a go at making that change? Feel like I'm asking a lot without actually contributing!

That's actually good idea, this might work. I'd be more than happy, if you can kick it out 🙏

andypearson commented 5 years ago

I've stared working on this during my commute, hopefully have a PR for you soon...

jukben commented 5 years ago

@andypearson Awesome! Looking forward, thank you for this. 💪 Feel free to ping me in the PR, I'll help you if needed.

andypearson commented 5 years ago

Okay, after serious poking I have come up with: https://github.com/webscopeio/react-textarea-autocomplete/compare/master...andypearson:fix-attempt-2

First thing: this feels very close to the metal, and that may be because of my lack of React knowledge. Should I attempt to roll back to an implementation that uses setState? Personally, I prefer avoiding the chance of infinite loops :D

I've managed to remove the fixed width from the autocomplete and have switched height to max-height with overflow-y: auto;

I switched to using getBoundingClientRect because it seems to be more consistent in the target browsers, and you can totally avoid using x and y for IE (although I have not tested in IE yet). In my testing, it appears to handle various scroll positions seamlessly.

I ended up needing to use getComputedStyle to measure the margin on the autocomplete and then include that in the calculation to work out if the autocomplete was overflowing the container. I'm not sure if the original implementation has that bug but mine certainly did! Perhaps switching back to getClientRects might avoid this.

Do you mind pulling this down for a quick review? If you're happy with the overall approach, I'll fix up the tests and then submit a PR.

jukben commented 5 years ago

Hell yeah, @andypearson I will do it during this weekend! Thanks for this! 💪

jukben commented 5 years ago

@andypearson that actually works like charm, I'm impressed. 🚀 Great job. It looks like you just need to polish the code for linter, regenerate snapshots for Jest and we are ready to go (I can help you with that, once you open PR). I even did some tests in IE11 and still without any hassle. 👏

I ended up needing to use getComputedStyle to measure the margin on the autocomplete and then include that in the calculation to work out if the autocomplete was overflowing the container. I'm not sure if the original implementation has that bug but mine certainly did! Perhaps switching back to getClientRects might avoid this.

Not sure about this. It may be that the bug was in the original implementation as well. But overall I'm totally OK with your approach, seems pretty stable and for this type of job the best.

andypearson commented 5 years ago

Great stuff! Thanks for reviewing it and glad it's working nicely (even in IE11!)

I'll have a go at getting everything green for the PR in the next couple of days as it'd be good for me to understand this stuff. I'll give you a shout if I run into anything I can't figure out :)

benoitkopp commented 5 years ago

Hey guys, do you know when you release your great fix ? A user of my app just told me that her page crashed and I just figured out that she had this in the console: bug_charlie

Do you confirm that it is on your side ? I'll just wait for the fix :)

jukben commented 5 years ago

Hey, @BenoKop I've created an issue #125 let's discuss it there. 🙏

jukben commented 5 years ago

Deployed as 4.2.3. Thanks!