Skip to content
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

add setCursor on change in Editable #637

Closed

Conversation

pushkov-fedor
Copy link

@pushkov-fedor pushkov-fedor commented May 7, 2020

Issue #540
@raineorshine Raine, these days i was thinking a lot about this issue and trying different options. These 2 lines of code make sorting working in someway, but break backspace logic and cause some bugs. I think i should have published it earlier in order to get different feedbacks and opinions, sorry for that. :( The task is exciting and i'm ready to continue working on it, also it gave better understanding of current codebase, but takes a lot of time.

As i see, we just need to re-execute getThoughtsSorted in Subthought compoentnt and set the cursor back, but the devil in the details
I also tried dispatching setCursor in onChange method of Editable component, it sorted thoughts but Editable component lose focus after throttled thoughtChangeHandler function invocation. Maybe the problem is that value of Thought changed.

What do you think about it?

@raineorshine
Copy link
Contributor

The task is exciting and i'm ready to continue working on it, also it gave better understanding of current codebase, but takes a lot of time.

Good, I'm glad to hear that. This is challenging, but getting more familiar with the data model and codebase is great.

These 2 lines of code make sorting working in someway, but break backspace logic and cause some bugs.

It's kind of hard to tell with just that small change. Is it the wrong approach, or does it just need a lot of fixes? It breaks a lot of behavior from what I can tell. I'm concerned that it breaks fundamental invariants of the throttled editing logic.

As i see, we just need to re-execute getThoughtsSorted in Subthought compoentnt and set the cursor back

The cursor should still be valid, it's the browser selection that needs to be moved back.

I raised concerns about being able to restore the browser selection without it breaking editing. It may be worth trying though, since direct DOM manipulation is problematic as Bijay pointed out.

I suggest we invest only 3-4 hours to this issue. If this feature has higher priority then we can invest more . But if it takes lot of effort and major architectural changes , then we should put it in hold. We can reopen if we feel #503 will take much longer to address this issue.

I agree with Bijay on this. How much time have you put in so far, and what else you try within that window?

@pushkov-fedor
Copy link
Author

I spent 4 hours for this, figuring out what is going on by react/redux-dev-tools. I'd spent a lot of time trying to figure out, why after re-setting cursor we loose browser selection of Thought. Also I'd tried re-render Subthoughts component through callback from Editable component, but it needs more complex approach. I tried to use Sort button logic for triggering sorting function. All of these don't look like an acceptable solution, but i tried to find point to start.

@raineorshine
Copy link
Contributor

Thank you for digging in and trying out some possibilities. I think given the time investment it would be a good idea to put this on hold. We can look into this again after #503.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants