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

Sort thought during editing #540

Closed
raineorshine opened this issue Apr 12, 2020 · 9 comments
Closed

Sort thought during editing #540

raineorshine opened this issue Apr 12, 2020 · 9 comments
Labels
bug Something isn't working

Comments

@raineorshine
Copy link
Contributor

When editing a thought in a sorted context, it should be dynamically sorted to the correct place in real-time.

This must be done without interrupting editing, i.e. losing the browser selection.

@raineorshine raineorshine added bug Something isn't working dogfood labels Apr 12, 2020
@raineorshine raineorshine changed the title Sort edited thought dynamically Sort thought during editing Apr 23, 2020
@pushkov-fedor
Copy link

@raineorshine I work on solution, but there's a moment i would like to clarify. Loos like we need to rerender Subthoughts component while editing. In Editable component dispatching render action or calling setCursorOnThought function sort thoughts, but cause some unnecessary side effects. What do you think about it? Is it correct way to start solving this issue? I'm just a little confusing about thoughtsRanked, contexts, thoughtsResolved entities and their impact on render :)

@raineorshine
Copy link
Contributor Author

raineorshine commented Apr 28, 2020

Loos like we need to rerender Subthoughts component while editing.

This is a tough decision, which I'm quite ambivalent about. We can't re-render the Subthoughts component of a thought we are editing because we would lose the browser selection. I don't trust that we can restore the browser selection without interrupting the user's typing, which is not acceptable.

I'm almost tempted to do direct DOM manipulation. As bad as that is, it might be the least disruptive, and it will get re-rendered in sorted order anyway.

I'm not sure which approach has the most risks.

In Editable component dispatching render action or calling setCursorOnThought function sort thoughts, but cause some unnecessary side effects.

Yes

What do you think about it? Is it correct way to start solving this issue?

It's very hard for me to tell. I'm no React expert. Maybe @shresthabijay can offer an opinion.

I'm just a little confusing about thoughtsRanked, contexts, thoughtsResolved entities and their impact on render :)

That's understandable. There is a steep learning curve. Let me know know if there's anything specific about the architecture I can explain for you. Although the documentation is quite sparse, it might be worth going over Understanding the Data Model.

@shresthabijay
Copy link
Contributor

It's very hard for me to tell. I'm no React expert. Maybe @shresthabijay can offer an opinion.

It's quite tricky. I tried re-rendering Subthoughts and it is not causing loss of input focus but it definitely is causing weird cursorOffset behviour. But we can possibly solve that by storing cursorOffset within component, so after a re-render it can restore the offset properly. But still current architecture is made in a way that doesn't favor re-rendering top level sub thoughts on edit.

@shresthabijay
Copy link
Contributor

I'm almost tempted to do direct DOM manipulation. As bad as that is, it might be the least disruptive, and it will get re-rendered in sorted order anyway.

Yes it's tempting but we should avoid these hacks as much as we can. It will cause lot of troubles later.

@shresthabijay
Copy link
Contributor

@raineorshine sorting thoughts in a context just abruptly changes order without animation. Will this be a good experience in real time while user is typing ?

@shresthabijay
Copy link
Contributor

@pushkov-fedor Let me know if you need any help. If you can send a draft PR of your progress on this issue, I can test it and see if I can provide some solution.

@raineorshine
Copy link
Contributor Author

But still current architecture is made in a way that doesn't favor re-rendering top level sub thoughts on edit.

@shresthabijay Do you think we should wait until #503 before doing this?

sorting thoughts in a context just abruptly changes order without animation. Will this be a good experience in real time while user is typing?

Animation will make it better, but I think even without animation it will be a better experience than what we have currently. I use sorted lists in em and my experience so far is that when the subthoughts re-render at the end it is disorienting because the thought just edited suddenly moves. If the list was kept sorted while editing, the most abrupt change would happen on the first character press, but the cursor and browser selection would still be on the item so it will be easy to track.

Also, having an intermediate state where the list is unsorted breaks the invariant that a sorted list should always be sorted. Stronger invariants yields a more stable territory (more reliable affordances) for the user.

@shresthabijay
Copy link
Contributor

shresthabijay commented Apr 29, 2020

@shresthabijay Do you think we should wait until #503 before doing this?

We sure will have different implementation of this probably with animation.

I tested earlier today by re-rendering the component on edit. Input didn't loose it's focus but cursorOffset was not working properly. If we could solve that we can re-render on edit by calling sort thoughts for thoughts that are children of sorted context.

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.

@raineorshine
Copy link
Contributor Author

Believe it or not, this is actually working now! Lucky us!

@raineorshine raineorshine removed the hold Pause development label Jan 24, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

3 participants