-
Notifications
You must be signed in to change notification settings - Fork 110
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?
Conversation
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 is a good start, thanks!
You may want to address the behavioral issues below, and then we can move on to refactoring to clean up the design.
1. False negative: hovering slightly above to Colorado
There is one false negative for the bottom arrow. It doesn't show the indicator arrow even when hovering over the sorted context. Here I am hovering Colorado
over the Wyoming bullet (which will drop it into the sorted States
context):
2. False positive: hovering over child of sorted context
There is a false positive for both top and bottom arrows. The indicator arrow is incorrectly shown when dragging into a child of the sorted context, in which the DropHover is not outside the viewport:
The other thing to test is that it works on mobile when the keyboard is up. Unfortunately position: fixed
does not work when the keyboard is up, so you'll have switch to position: absolute
like in https://github.com/cybersemics/em/blob/587e1d309d9570e5ad06e9bec8eb0570867aa7d4/src/components/Tutorial/TutorialScrollUpButton.tsx.
src/components/Content.tsx
Outdated
// The arbitrary space above the first thought | ||
const spaceAbove = 100 |
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 space is not arbitrary. What happens when the styles change and the space is altered? Do not treat dependent variables as independent variables. If this is dependent on the actual space above the first thought, then it needs to be functionally linked.
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.
Sorry about this, I wanted to calculate the space relative to font size and toolbar height but left this in there accidentally.
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.
No problem! Glad you're aware.
src/components/LayoutTree.tsx
Outdated
// When a thought is hovered over a sorted context, determine the position of arrow. | ||
useEffect(() => { | ||
calculateThoughtPosition(treeThoughtsPositioned) | ||
}, [calculateThoughtPosition, treeThoughtsPositioned]) |
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.
Could you explain the trouble you had putting this in LayoutTree
? It really shouldn't go in Content
. Maybe we can work through the problems you encountered.
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 first put it in LayoutTree
but the arrow was not staying fixed relative to the viewport during scroll. So I put it in Content
, I really don't like this way as well since I have to pass props to LayoutTree
which really does not need to be accepting 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.
It's going to have to be switched to position: absolute
anyway to make it work on mobile Safari.
src/components/Content.tsx
Outdated
@@ -47,6 +55,66 @@ const Content: FC = () => { | |||
return children.length | |||
}) | |||
const isAbsoluteContext = useSelector(state => isAbsolute(state.rootContext)) | |||
const visibilityRef = useRef<'above' | 'below' | null>(null) |
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.
Refs are used to persist state across renders. Are you sure this should be a ref? It seems to me that the indicator arrow state would be completely determinable from the existing state. If it is a function of the state at one point in time, that it can be captured in a useSelector
or useMemo
.
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 did not use state because it would cause too many renders during drag but if it can be calculated with existing state maybe I need to think about it again.
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.
useRef
and useMemo
can both be used to persist state across render, but for different reasons.
useRef
is used when you need different information from the last render (e.g. the previous cursor location).
useMemo
is used when you need the same information from the last render (for performance optimization).
src/components/Content.tsx
Outdated
const monitor = dragDropManager.getMonitor() | ||
|
||
const item = monitor.getItem() |
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.
These should go inside the useSelector
scope.
src/components/Content.tsx
Outdated
const item = monitor.getItem() | ||
|
||
const sourceThought = useSelector(state => { | ||
const sourceThoughtId = head(item?.path || []) |
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 should probably add a check to make sure the dragged item is a Thought and not a ToolbarButton.
src/components/Content.tsx
Outdated
/** Calculate the visibiltiy of thought being dragged in a sorted context. */ | ||
const calculateHoverArrowPosition = useCallback( | ||
(thoughts: TreeThoughtPositioned[]) => { | ||
const state = store.getState() |
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.
Getting the state directly from the store is usually incorrect, as it can go stale. You need useSelector
to ensure your derived values are updated when the state changes.
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.
That's right, but I can't use useSelector
inside a function so I turned to using getState
method.
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 would need to add useSelector
outside the function, or turn the whole callback into a useSelector
.
src/components/Content.tsx
Outdated
/** Calculate the visibiltiy of thought being dragged in a sorted context. */ | ||
const calculateHoverArrowPosition = useCallback( |
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.
How much of this can be precalculated in treeThoughtsPositioned
? The purpose of treeThoughts
and treeThoughtsPositioned
is to have two distinct rendering phases. The first linearizes the tree, and the second calculates the x,y positions based on the size measured in the DOM. Performing additional nontrivial calculations outside these two phases adds additional complexity. It's like we're adding a third rendering phase, but it occurs in a useEffect
.
linearizeTree
and the useMemo
that generates treeThoughtsPositioned
has access to most of the state and layout information needed determine the sort position of rendered thoughts. The only thing it might not have access to is the drag-and-drop state, but you should be able to access the drag-and-drop monitor there just as easily?
Another way to put it: We are already iterating through all the tree thoughts in both linearizeTree
and the useMemo
that generates treeThoughtsPositioned
(I really need to move that into a custom hook and give it a better name). Thus, why are we iterating through all of the tree thoughts again? It would be more efficient and less complex to perform these calculations within one of those original loops—the two phases of LayoutTree rendering.
Hope that makes sense! Let me know if I can clarify anything.
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 understand the concern of reiterating the treeThoughtsPosition
array, if I can put the arrow in the LayoutTree
without scrolling it out of the viewport, we can use the existing state to remove the unnecessary iterations. Maybe I missed something I will try again with the arrow placement and try to find a fix for this.
src/components/Content.tsx
Outdated
const currentThought = getThoughtById(state, head(path)) | ||
lastSortedThought = isSortedContext ? thought : null | ||
|
||
if (isSortedContext && Math.floor(newRank) === currentThought.rank) { |
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.
Ranks are supposed to be fractional, so you should never floor
them. e.g. This will break if there are thoughts with ranks 1.1, 1.2, and 1.3, because they all floor to 1.
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.
All of this new logic should probably go in a custom hook as well to separate it cleanly from the rest of the component logic.
Looking good! @Zubair286 - Have you been able to resolve this?
I am still seeing it locally during testing: Screen.Recording.2024-09-18.at.1.23.52.AM.mov |
@trevinhofmann I am not sure if its a false positive but it still thinks its in Screencast.from.09-18-2024.11.39.58.AM.webm |
I wonder if its because its hovering over |
@raineorshine in this case where we are hovering over |
Can you provide steps? I'm not sure what you mean. The arrow should be shown whenever the drop target is outside the viewport. |
I meant like in this case, where I am hovering over Screencast.from.09-18-2024.11.34.47.AM.webm |
Yes, this case should show the arrow. Any hover that drops into |
Understood, I don't believe we have any selector that tells us that we are hovering over a drop end, since this is only because of that rest of the case are working fine. |
@raineorshine @trevinhofmann can you please review and confirm the functionality of this PR? Thanks |
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.
Thank you @Zubair286! The functionality all appears to work as expected when I test it locally, including the edge case I mentioned previously.
I've added a few comments, mostly related to refactoring that should be implemented to avoid polluting the <LayoutTree>
with this new code which should be encapsulated elsewhere.
src/App.css
Outdated
50% { | ||
transform: translateX(-50%) translateY(10px); | ||
} | ||
} |
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.
My understanding has been that we want to avoid adding anything to App.css
in favor of completely switching to Panda for anything new, but the bobble animation here looks fine to me so long as @raineorshine is okay with this.
If you switch to Panda, here is an example of a pulse
animation defined in panda.config.ts
:
Lines 35 to 42 in 66945ae
pulse: { | |
from: { | |
opacity: 1, | |
}, | |
to: { | |
opacity: 0.25, | |
}, | |
}, |
I see you have already defined the arrowBobbleAnimation
in panda.config.ts
, so adding the animation keyframe there as well would be my preference for consistency.
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.
Sure, I will add them in panda.config.ts
src/App.css
Outdated
|
||
.copy-icon-wrapper svg { | ||
cursor: pointer; | ||
} |
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.
Were these checkbox, gift code, and copy icon styles unintentionally added? I don't see how they relate to the pull request, so I assume they were accidentally included when resolving a merge conflict (or something similar).
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.
Yes, I believe so they got added during merge conflict during a rebase. I will remove them.
src/components/LayoutTree.tsx
Outdated
@@ -449,22 +484,51 @@ const LayoutTree = () => { | |||
[fontSize], | |||
) | |||
|
|||
/** Compare ranks between two thoughts. */ |
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 is a pure function and can/should be defined outside the context of the LayoutTree
component (or any component).
src/components/LayoutTree.tsx
Outdated
animation: `bobble ${token('durations.arrowBobbleAnimation')} infinite`, | ||
}} | ||
></div> | ||
)} |
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 recommend breaking this into a separate component with its own name such as <HoverArrow>
. Being defined inline here without a name makes the intended functionality non-obvious to anyone skimming through the code.
@trevinhofmann can you review the latest code refactor changes? Thanks |
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.
The refactor looks much better, thank you!
3. False positive when dragging over toolbar or footer
@raineorshine -- I noticed one edge case. If the user drags the thought into either the toolbar or footer, the arrow is still displayed even though the thought does not get dropped into the list. Is that functioning as expected?
Demos of the edge case:
Screen.Recording.2024-09-24.at.8.23.14.AM.mov
Screen.Recording.2024-09-24.at.8.21.10.AM.mov
src/hooks/useSortedContext.ts
Outdated
/** A hook that checks if a dragging thought is hovering over a sorted context, and returns new rank where that thought will be dropped. */ | ||
const useSortedContext = () => { | ||
const hoveringPath = useSelector(state => state.hoveringPath) | ||
const contextParentPath = parentOf(hoveringPath || []) |
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.
@raineorshine - Would you agree that we should avoid ||
in favor of ??
for default cases like this throughout the project? I personally use the prefer-nullish-coalescing ESLint rule in my projects, because it only applies for undefined
and null
rather than all falsy values (such as 0
), which is generally the desired behavior for default values.
@Zubair286 - This is just a comment for discussion. You don't need to make this change now, since ||
is already used frequently throughout the project.
src/util/compareRanks.ts
Outdated
@@ -0,0 +1,23 @@ | |||
/** Compare ranks between two thoughts. */ |
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.
Is there a reason for the complexity in this function, where we convert the ranks to lists of numbers (separated by the decimal point) and compare those individually?
The function compareRanks
is only used in one place to check for a negative number (compareRanks(newRank, currentRank) < 0
), and it seems to return a negative number if and only if rankA < rankB
. I might be misunderstanding some edge cases that this handles; can you please explain why we can't compare the ranks simply as rankA - rankB
?
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 was not too sure if there could be ranks like 1.1.2, 1.1.3 etc, but given the possibility I had the function written like this. It certainly can be done with just rankA - rankB
, if there are not any ranks as above, please do correct me if I am wrong here.
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.
Since the parameters are of type number
, values such as 1.1.2
would not be possible.
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.
Hmm right that was thoughtless of me, not sure why I assumed it was string, but in that case we can omit the compareRanks
function and simple do newRank - currentRank
and this would work just fine.
Good catch. The arrow should only appear when hovering over a valid drop target. |
The current logic depends on |
Yes, I think you're right. |
Right, I guess we are gonna need to update its action to return |
I see Edit: The last hovered path is only shown when hovered over toolbar, on footer it shows the path of thought that footer is covering, that is strange I think. |
Comparing the last path with the current path doesn't really make sense to me. It seems like the drag-and-drop monitor item should know the hover path. |
Yes, I can confirm it is not a viable solution. The drag and drop monitor item has the information of the dragged item not the drop path I believe. |
Ah, good point. I'm not sure then off the top of my head. It's going to take some investigation. |
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.
When I tested, 1-7
and 9
are all working as expected.
However, I was able to reproduce 8. False positive when canDrop is false
:
Screen.Recording.2024-10-25.at.12.42.28.AM.mov
@trevinhofmann its strange I can not reproduce this, I have checked logs and the Screencast.from.10-25-2024.11.19.17.AM.webm |
Nevermind this I imported the context again and it reproduced. I have pushed the fix, can you confirm @trevinhofmann? |
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.
Thanks, @Zubair286! All of 1-9
are now working as expected for me.
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 have tested all the cases and can confirm they all work! Amazing job.
Now just some optimization and cleanup.
import useSortedContext from './useSortedContext' | ||
|
||
const DEBOUNCE_DELAY = 50 | ||
let hoverCount = 0 |
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'm not sure. Generally debouncedSetHoveringPath
is always called when hoverCount
becomes 0 anyway, per the previous else statement, as that is the only place that hoverCount
is decremented. In that case the cleanup function isn't doing anything. But if hoverCount
is never incremented, then it could possibly be 0 on unmount without debouncedSetHoveringPath
being called. As for what that state corresponds to or whether debouncedSetHoveringPath
needs to be called then I couldn't say.
src/hooks/useSortedContext.ts
Outdated
const dragDropManager = useDragDropManager() | ||
|
||
// get the source thought and its new rank | ||
const sourceThought = useSelector(state => { |
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.
It's recommended to combine everything into a single useSelector
when possible. Think "thick" selector. Otherwise the render function will be called when any of the intermediate values change. We only need it to re-render when the final value changes.
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.
src/components/LayoutTree.tsx
Outdated
return { indentCursorAncestorTables, treeThoughtsPositioned } | ||
}, [fontSize, sizes, singleLineHeight, treeThoughts]) | ||
// Determine hoverArrowVisibility based on newRank and the visible thoughts | ||
if (newRank > 0 && (isSortedContext || hoveringOnDropEnd)) { |
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.
src/hooks/useSortedContext.ts
Outdated
return { | ||
isSortedContext, | ||
newRank, | ||
hoveringOnDropEnd, |
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.
isSortedContext
and hoveringOnDropEnd
are only ever used in the expression isSortedContext || hoveringOnDropEnd
. Therefore, it would be more performant and better encapsulated to move the expression into the hook, despite what might become an awkward name.
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.
src/components/LayoutTree.tsx
Outdated
@@ -504,6 +540,7 @@ const LayoutTree = () => { | |||
const tableCol1Widths = new Map<ThoughtId, number>() | |||
const treeThoughtsPositioned = treeThoughts.map((node, i) => { | |||
const next: TreeThought | undefined = treeThoughts[i + 1] | |||
const state = store.getState() |
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 add rank
and isSortedContext
to TreeThought
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.
src/components/HoverArrow.tsx
Outdated
height: '0', | ||
borderLeft: '10px solid transparent', | ||
borderRight: '10px solid transparent', | ||
position: 'absolute', |
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 of position: fixed
? It seems like position: absolute
would involve a lot more manual calculation to get it in the viewport, while position: fixed
does that for you.
src/components/LayoutTree.tsx
Outdated
const toolbar = document.querySelector('#toolbar') | ||
if (toolbar) setToolbarHeight(toolbar.getBoundingClientRect().height) | ||
|
||
setDistanceFromTop((ref.current?.getBoundingClientRect().top ?? 0) + scrollTop) |
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 calculate maxVisibleY
. Therefore, you can remove scrollTop
from these calculations and avoid having to trigger useLayoutEffect
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.
Comments with commit hashes were minor issues that I fixed. I recommend taking a peek at the commits. That just leaves two questions: |
3cd0ef0
to
a828e1a
Compare
#1828
This issue implements a feature that adds an indicator arrow on top/bottom of the viewport, if a thought is dragged over a sorted context and its drop position is outside of viewport. Checking the rank of the target position and comparing the y value of that thought with viewport height + scrollY for bottom arrow, and comparing y with scrollY for top arrow.