Added sidebar stickiness, icon clicking #155
Conversation
src/lib/SideBar.svelte
Outdated
| let activeInfo = null; // Tracks currently visible info | ||
| let lastHoveredInfo = null; // Tracks last hovered info | ||
| let isPanelHovered = false; // Tracks if info panel is hovered | ||
| let hideTimeout = null; // Timeout for hiding info panel |
There was a problem hiding this comment.
I think timeouts are numerical ids, so we should initialize this with 0
src/lib/SideBar.svelte
Outdated
| function showInfo(info) { | ||
| activeInfo = info; | ||
| clearTimeout(hideTimeout); // Cancel timeout |
There was a problem hiding this comment.
We should reset hideTimeout to 0 to avoid clearing it multiple times. It might be also sensible to add an if(hideTimeout) before, unless we are sure the timeout is active
src/lib/SideBar.svelte
Outdated
| class="flex flex-col gap-5 shrink-0 w-80 h-full z-10 p-2 bg-neutral-600 text-gray-100 opacity-95" | ||
| class:hidden={!activeInfo} | ||
| on:mouseover={() => { | ||
| isPanelHovered = true; |
There was a problem hiding this comment.
Let's keep all the logic in functions above
src/lib/SideBar.svelte
Outdated
| function hideInfo() { | ||
| activeInfo = null; | ||
| hideTimeout = setTimeout(() => { | ||
| if (!isPanelHovered) { |
There was a problem hiding this comment.
How can this happen? We call clearTimeout when setting isPanelHovered to true
There was a problem hiding this comment.
I'm not very sure what you meant by this, but when we leave the panel, isPanelHovered is set to false. After the timeout got called and is over, the rest of variables get set to null and 0 which says that there is no hovering over the panel
There was a problem hiding this comment.
isPanelHovered can only be false here. When set to true we also call clearTimeout, so the code will never run in the first place.
src/lib/SideBar.svelte
Outdated
| /> | ||
| {:else} | ||
| <div class="grow" on:mouseover={(e) => showInfo(null)}></div> | ||
| <div class="grow" style="pointer-events: none;"></div> |
There was a problem hiding this comment.
Use tailwind CSS classes instead of a custom style
src/lib/SideBar.svelte
Outdated
| function hideInfo() { | ||
| activeInfo = null; | ||
| hideTimeout = setTimeout(() => { | ||
| if (!isPanelHovered) { |
There was a problem hiding this comment.
isPanelHovered can only be false here. When set to true we also call clearTimeout, so the code will never run in the first place.
src/lib/SideBar.svelte
Outdated
| activeInfo = null; | ||
| if (!hideTimeout) { | ||
| hideTimeout = setTimeout(() => { | ||
| if (!isPanelHovered) { |
There was a problem hiding this comment.
I still think this logic is redundant. Below we should call clear timeout so that the callback is never even called if isPanelHovered is set to true.
There was a problem hiding this comment.
I understand what you mean now, having the timeout outside of the if (!isPanelHovered) is redundant. I've placed it inside the if statement.
if (!hideTimeout) makes sure that when you hover too fast over an icon and the cursor moves out of the panel, you're still able to hover back within those 400ms so you can stay in the panel without it closing, otherwise it just closes the panel
I think keeping the if (!hideTimeout) would be nice for a more 'stickier' feeling but please let me know what you think about it.
src/lib/SideBar.svelte
Outdated
| hideTimeout = 0; | ||
| } | ||
| // Updates activeInfo and lastHoveredInfo to track the hovered icon, then shows the panel with the hovered icon's info | ||
| if (info !== lastHoveredInfo) { |
There was a problem hiding this comment.
I am not seeing the rationale for lastHoveredInfo here. Maybe at some point in the past we had some grace time before switching from one panel to another?
| function hideInfo() { | ||
| activeInfo = null; | ||
| if (!hideTimeout) { |
There was a problem hiding this comment.
Can hideTimeout ever be non-zero here? I think we reset the timeout when the panel is hovered, which means it will always be 0 when mouseleave is reached. Potentially we could use mouseenter to make this more obvious?
There was a problem hiding this comment.
@AtibQur Please check if this condition is redundant. It's the last open issue.
There was a problem hiding this comment.
@alexp-sssup We need to use this condition.
Without this condition, we are not able to keep the panel open when we accidentally hover out and back in to the panel.
There was a problem hiding this comment.
In handleMouseEnterPanel the timeout is cleared. I am not seeing how hideTimeout can be non-zero here.
There was a problem hiding this comment.
hideTimeout can be non-zero because handleMouseLeavePanel (via hideInfo) starts a timer. When showInfo or handleMouseEnterpanel clears the timer, hideTimeout still holds the timer ID until it resets to 0. During this moment, hovering over the panel will cancel the timer before it can trigger, preventing the panel from closing.
I hope my explanations made a bit of sense, please let me know if it's still unclear.
There was a problem hiding this comment.
clears the timer, hideTimeout still holds the timer ID until it resets to 0.
Are you referring to a concurrency problem here? JavaScript is not concurrent. hideTimeout is reset to 0 immediately after clearTimeout. No other event or action can happen till the previous handler is complete.
| function hideInfo() { | ||
| activeInfo = null; | ||
| if (!hideTimeout) { |
There was a problem hiding this comment.
@AtibQur Please check if this condition is redundant. It's the last open issue.
| function hideInfo() { | ||
| activeInfo = null; | ||
| if (!hideTimeout) { |
There was a problem hiding this comment.
In handleMouseEnterPanel the timeout is cleared. I am not seeing how hideTimeout can be non-zero here.
No description provided.