Fix auto select in InspectDataMenu when opened above cursor #691
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
There was a problem hiding this comment.
I don't think this is the right fix for the issue. Instead of working around button events, I believe it should be enough to offset the menu such that it appears over the cursor rather than directly under. This would also be better from UX perspective, as now, the menu covers the element we are inspecting, which isn't the case when the menu displays under.
I think we should change from using an outer-level div for positioning, to adjusting the position of the Trigger component. This seems to be the way based on the examples from https://www.radix-ui.com/primitives/docs/components/dropdown-menu – right now the trigger component is empty while it seems the offset is added automatically such that the menu doesn't overlap the trigger.
Menu shown under:
Menu shown over:
p-malecki
force-pushed
the
@p-malecki/fix-menu-close-above-cursor
branch
from
b077f83
to
3034908
Compare
…reverted" This reverts commit 91d3479.
There was a problem hiding this comment.
Looks good. As a final touch you may want to change the logic slightly such that if there are exacly LIMIT+1 elements, you still show them all, as LIMIT+1 items occupy exactly the same amount of space as LIMIT + "show more" button. So when you click "show more" you don't actually expand the menu at all.
| left: inspectLocation.x, | ||
| top: inspectLocation.y, | ||
| opacity: 0, | ||
| }}></span> |
There was a problem hiding this comment.
| }}></span> | |
| }}/> |
| </DropdownMenu.Label> | ||
| {inspectItems.map((item, index) => { | ||
| // extract file name from path: | ||
| const fullFileName = item.source.fileName.split("/").pop() ?? ""; |
There was a problem hiding this comment.
I would extract it to separate Component or at least helper for the strings
| return ( | ||
| <DropdownMenu.Item | ||
| className="inspect-data-menu-item" | ||
| key={index} |
There was a problem hiding this comment.
In general, index as a key is a bad practice, I think in our case when we don't have state it shouldn't cause bugs, but maybe you could use something as unique ID here? For example filePath with code line
| </DropdownMenu.Item> | ||
| ); | ||
| })} | ||
| {filteredData.length > MAX_INSPECT_ITEMS + 1 && !shouldShowAll && ( |
There was a problem hiding this comment.
extracting this condition to some variable would improve readability
|
|
||
| interface InspectItemProps { | ||
| item: InspectDataStackItem; | ||
| onSelected: (item: any) => void; |
There was a problem hiding this comment.
Why any? Isn't it InspectDataStackItem?
| {isOverMaxItems && !shouldShowAll && ( | ||
| <DropdownMenu.Item | ||
| className="inspect-data-menu-item show-all" | ||
| key={"show-all"} |
There was a problem hiding this comment.
I think now the key is unnecessary as the elements here differ
There was a problem hiding this comment.
Its needed to avoid warrning [WEBVIEW LOG] Warning: Each child in a list should have a unique "key" prop.%s%s in logs.
| } | ||
|
|
||
| const InspectItem = React.forwardRef<HTMLDivElement, InspectItemProps>( | ||
| ({ item, onSelected, onHover }, forwardedRef) => { |
There was a problem hiding this comment.
Why forwardRef? As far as I can tell, it's not used
There was a problem hiding this comment.
I think it's the way for creating custom components in radix https://www.radix-ui.com/primitives/docs/components/dropdown-menu
|
|
||
| const InspectItem = React.forwardRef<HTMLDivElement, InspectItemProps>( | ||
| ({ item, onSelected, onHover }, forwardedRef) => { | ||
| const extractFileDetails = (filePath: string) => { |
There was a problem hiding this comment.
Imo, now you don't need a helper (I meant component or helper, but I prefer the component) - just fyi, when you define a method in render, it's created on every rerender
This PR addresses an issue where the inspector context menu automatically selects and closes upon appearing above the cursor. The problem was caused by the menu opening upon mouseUp immediately after a mouseDown event.
The solution ensures the menu displays adjacent to the cursor with an offset by introducing a
spanthat positions theTriggercomponent at the mouse location, ensuring the menu opens next to theTrigger.Additionally, this PR:
InspectDataMenuto 5 usingMAX_INSPECT_ITEMSInspectDataMenuFixes #687
How Has This Been Tested:
After:
Screen.Recording.2024-11-05.at.18.31.10.mov