Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 indicator arrow on top/bottom of viewport for drop positions outs… #2369
base: main
Are you sure you want to change the base?
Add indicator arrow on top/bottom of viewport for drop positions outs… #2369
Changes from 10 commits
fac864b
67b555c
44939c6
9974579
2e0b0f2
fcc2a23
8b14939
214a3d4
25c6ba9
0ae9219
18de5b1
a3d251f
a47d585
25a03e3
853a23d
40a456f
c8a1be5
8192dfc
8f4aada
a828e1a
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why
position: absolute
instead ofposition: fixed
? It seems likeposition: absolute
would involve a lot more manual calculation to get it in the viewport, whileposition: fixed
does that for you.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You add
scrollTop
here only to subtract it below. i.e. It cancels out when you calculatemaxVisibleY
. Therefore, you can removescrollTop
from these calculations and avoid having to triggeruseLayoutEffect
on scroll.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
8f4aada
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We want to avoid importing the store directly. Calling
getState()
outside of a selector can result in a stale state.The first render phase,
linearizeTree
, has access to a fresh State, we we can addrank
andisSortedContext
toTreeThought
there and pass them down to the positioning phase.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
c8a1be5 and 8192dfc
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rank is an arbitrary integer that can be negative, so the comparison with 0 is not a good idea. For example, you could have a sorted list with ranks [-3, -2, -1].
In the current codebase, when a context is sorted it may be the case that thoughts get re-ranked starting from 0, but this is tenuous and implementation-specific so we should avoid relying on that.
I can't quite figure out what purpose
newRank > 0
is serving, so maybe it can be removed. Otherwise let me know what the intention is and we can find a different way to accomplish it.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This may be necessary, but it feels convoluted to have so many variables going into the calculation of arrow positioning. Feel free to disregard this if there isn't a simpler approach, but I suspect there is one.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do feel that there are a lot of variables involved but I can't see a proper positioning of the arrow without using them.