-
Notifications
You must be signed in to change notification settings - Fork 0
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
Implement front-end routing #19
Conversation
export const findNodeInTree = ( | ||
tree: TreeNode[], | ||
itemId: string, | ||
): { | ||
found: TreeNode | undefined; | ||
path: TreeNode[]; | ||
} => { | ||
const path: TreeNode[] = []; | ||
|
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 refactor adds the ability to do a lookup in a tree by item id, which wasn't something was needed before (the user was clicking on the tree to navigate to items). We need this for routing to be able to go from a path to somewhere arbitrary in the tree.
65d0456
to
038f0c0
Compare
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.
overall looks good, just a few things 👍
displayedRowKey, | ||
) as DisplayedListItemRefAndSetter; | ||
const route = useRoute(); | ||
watch( |
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.
what's going on in this file? Also I don't love the idea of subcomponents being able to navigate, is it possible to abstract this to the main component?
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 moved this up one component to <ListTree>
where the rest of the list tree state is owned.
What's going on is that the <ListTree>
needs to react to route changes:
- tree rows need to expand and contract
- tree rows need to be selected
Also I don't love the idea of subcomponents being able to navigate, is it possible to abstract this to the main component?
I guess I wouldn't say that the subcomponent is navigating: it's just reacting and manipulating its own state in response to route changes. I gave it a little thought, and I don't think it makes sense to put the tree expanded and selected keys state on the top-level component. I'd rather adopt a Redux/Pinia store pattern before doing that. The client state management would be a little more compact if we were using Pinia.
You'll notice the main component is in fact where router.push()
happens.
Let me know if I can clarify this with variable naming or comments, or if you have other suggestions.
2bb753c
to
43d07c9
Compare
8b47f1a
to
3754fb6
Compare
43d07c9
to
01a01fe
Compare
Not that it needs to be answered here, but an interesting question around frontend routing is how to handle it in Lingo. We can assume it will have its own frontend routes, does that place nicely with frontend routing for |
48d8ffb
to
f28144e
Compare
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.
looks good, save a few minor nits
arches_references/media/js/views/components/plugins/controlled-list-manager.js
Outdated
Show resolved
Hide resolved
}; | ||
|
||
// Navigate on initial load of the tree. | ||
watch( |
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.
nit: at least have the two watch
es grouped together. Any chance this could be refactored to use onMounted
instead?
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.
Grouped together. Re: onMounted, that's an interesting idea, although I confess to not liking the lifecycle methods as a plan A (feels like pre-react-hooks to me)--but I'd rather explore it as part of a refactor to pull out the functionality that fetches lists and put that state somewhere a little more logical.
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.
lgtm 👍
4838262
to
78681c6
Compare
Natural forward and back navigation is now supported when interacting with lists & items. Allows resolving the default item URIs generated in #17.
Closes #4