-
Notifications
You must be signed in to change notification settings - Fork 49
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
feat: make selection node mechanism route based #2659
base: master
Are you sure you want to change the base?
Conversation
hey @KVV94 any update on the failed tests? |
Just fixed the unit tests. They are passing successfully now. |
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.
Nice work, please take a look to my comments
import { useModal } from '~/stores/useModalStore' | ||
import { useUserStore } from '~/stores/useUserStore' | ||
import { NodeExtended } from '~/types' | ||
import { executeIfProd, getLSat } from '~/utils' | ||
import { SuccessNotify } from '../common/SuccessToast' | ||
import { useNodeNavigation } from '../Universe/useNodeNavigation' |
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.
use absolute 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.
done
@@ -7,6 +7,7 @@ import { useDataStore, useNodeTypes } from '~/stores/useDataStore' | |||
import { useGraphStore, useHoveredNode, useSelectedNode } from '~/stores/useGraphStore' | |||
import { NodeExtended } from '~/types' | |||
import { colors } from '~/utils' | |||
import { useNodeNavigation } from '../../useNodeNavigation' |
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.
use absolute 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.
done
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.
should not we add routing for non mindset part too ?
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've just added the routing for the non mindset part as well. Thank you for noticing this
Ticket № 2645
closes #2645
Problem:
Currently, the selected node state is managed through application state, making navigation less predictable and hindering the use of standard browser features (like the Back button). We need to make the node selection mechanism route-based.
Solution:
Implemented a route-based node selection mechanism through a new useSelectedNodeFromUrl hook that synchronizes URL state with the selected node state. This makes node selection more predictable and allows for better browser integration
Changes:
Testing:
useNodeNavigation tests:
useSelectedNodeFromUrl tests:
Notes:
Using replace: true in navigation to prevent unnecessary browser history entries
No state changes occur when node ID from URL is not found
Maintained backward compatibility with existing node selection mechanism
Browser navigation (back/forward) now works seamlessly with node selection