webscopeio / react-textarea-autocomplete

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

Getters and setters for caret position #28

Closed marcinlichwala closed 6 years ago

marcinlichwala commented 7 years ago

Caret position as integer (not coordinates). Test for entering data not working properly yet, might need help with this one.

jukben commented 7 years ago

Good job so far. As you mentioned setCaretPosition is duplicate of setTextareaCaret. We should refactor setTextareaCaret instead. Also, I would like to propose that we should rename all "private" methods following the pattern _method and maybe use some comments notation above each section of "private" and "public" methods. What do you think?

Next thing what missing is onCaretPositionChange={(position: integer, ref: React$Class) => void}} prop. I think that could be really handy. Could you implement this one too, I think you miss it in my description in issue.

Anyway, GJ, thanks! ❤️

I'll look on the test after you manage these changes. I think there is some problem with emulation of caret itself, but don't worry about it for now. 🙏

marcinlichwala commented 7 years ago

Ok, so we will think together about this caret simulation because it would be valuable to test it. Mount should be using JSDom, but maybe it doesn't work well enough in this scenario. Maybe this is too much outside the DOM and too much in the browser internals? In that case we would rather need some e2e tests for this?

The plan for me now is:

jukben commented 7 years ago

@marcinlichwala basically each of them except these two is "private". 🙈

I'm scared that it's out of the scope of JSDom. I will create better example - inside of repo, for better dev experience but also for proper E2E testing via Cypress.io.

Thanks for your help!

P.S: I've updated description for you. 🤓

jukben commented 7 years ago

Hi @marcinlichwala what's the status? Should I assign this to someone else?

marcinlichwala commented 7 years ago

Hello @jukben, I would like to work on it this week, will give you status until Friday, would that work for you?

jukben commented 7 years ago

@marcinlichwala Yeah, cool. You have to rebase a lot because I've pushed new version (check it out, it's cool!). I introduced yarn dev for local development and added Cypress for E2E testing. :)

marcinlichwala commented 7 years ago

Wow, that sounds good. Never worked with cypress before, happy to test :)

marcinlichwala commented 7 years ago

@jukben Hello! Sorry for the delay. Unfortunately I'm having an ESLint problem with this underscore marking of private methods: image

Should I remove this check in the .eslintrc or locally in the Textarea.jsx?

jukben commented 7 years ago

@marcinlichwala Oh, please remove this rule in .eslintrcglobally. Thanks that you working on it. 💪

marcinlichwala commented 7 years ago

@jukben I think all the points from the checklist are covered now. Please check :)

jukben commented 7 years ago

@marcinlichwala Good job. Just last thing, could you please get the repo in sync? Rebase it to resolve the conflicts? It blocks the pipeline.

marcinlichwala commented 7 years ago

@jukben rebased with upstream/master, please check now :)

marcinlichwala commented 7 years ago

@jukben I've added the remaining docs with an example, please review :)

jukben commented 7 years ago

Hey, @marcinlichwala looks awesome! I've also enabled CircleCI on fork PR, so even CI approves the PR. 🎊

Am I just wondering about the wrapper in the test? Have you tried to change it? Did it work?

marcinlichwala commented 7 years ago

Woohoo! Will check the wrapper removal :)

marcinlichwala commented 7 years ago

@jukben Removed the wrapper class declaration and the ref prop from the rtaComponent because in the test scenario I still had to use the .instance() call on the ReactWrapper to get the correct ref :(

jukben commented 7 years ago

@marcinlichwala I think you are correct. Nice work! 🙏 What do you think about this one? https://github.com/webscopeio/react-textarea-autocomplete/pull/28#discussion_r149045882

marcinlichwala commented 6 years ago

Hello @jukben, finally had time to finish this one. I think we are getting there. Please check :)