Dynamic nodes prevent navigation tree mutation #321
Conversation
maxmarkus
commented
Jan 8, 2019
maxmarkus
commented
- navigation tree does not get mutated anymore
- substituted values are stored in routing component as before
- added tests for two dynamic segments
This reverts commit ec5f47c.
…nsaved-changes-modal # Conflicts: # core/src/services/routing.js
…nsaved-changes-modal
| @@ -123,6 +100,42 @@ export const buildRoute = (node, path, params) => | |||
| ? path + (params ? '?' + params : '') | |||
| : buildRoute(node.parent, `/${node.parent.pathSegment}${path}`, params); | |||
|
|
|||
| export const processDynamicNode = (node, pathParams) => { | |||
There was a problem hiding this comment.
This should be eliminated as it mutates the given node.
There was a problem hiding this comment.
After this has been removed the way the navigation links are built must be reworked (routing.js handleRouteClick(node) must take the current navigation path from the URL into consideration to properly deal with dynamic path segments in the current navigation path; currently this function builds the navigation path from scratch based on node structure alone which won't work with unmutated navigation nodes any more).
| return buildNode( | ||
| nodeNamesInCurrentPath, | ||
| const navObj = await buildNode( | ||
| [...nodeNamesInCurrentPath], // spread operator, because buildNode is mutating this input |
There was a problem hiding this comment.
Please avoid mutation in `buildNode' method and then we don't need to use the spread operator
There was a problem hiding this comment.
Thanks, I forgot to add the // TODO :) so it slipped through.
| [rootNode], | ||
| rootNode.children, | ||
| rootNode.context || {} | ||
| ); | ||
| navObj.isExistingRoute = | ||
| !activePath || | ||
| nodeNamesInCurrentPath.length == |
| '/users/groups/:group/settings' | ||
| '/users/groups/:group/settings', | ||
| children: [ | ||
| { |
There was a problem hiding this comment.
This doesn't look nice when clicked. I would recommend using the same approach as for the groups dynamic node
There was a problem hiding this comment.
I added keepSelectedForChildren: true, since we are not able to drill down here, because the child is dynamic. Think this is what you meant.
…ject substitution for context and other possible objects
…vent-navigation-tree-mutation # Conflicts: # core/examples/luigi-sample-angular/src/assets/extendedConfiguration.js
core/src/services/routing.js
Outdated
| @@ -264,6 +284,6 @@ export const handleRouteClick = node => { | |||
| navigateTo(link); | |||
| } else { | |||
| const route = RoutingHelpers.buildRoute(node, `/${node.pathSegment}`); | |||
| navigateTo(route); | |||
| navigateTo(RoutingHelpers.substituteViewUrl(route, componentData)); | |||
There was a problem hiding this comment.
To use the RoutingHelpers.substituteViewUrl is confusing because it is not applied on a view Url but on the main luigi navigation path. It works but it is - confusing - and dangerous because we might change the substituteViewUrl in the future and not think that it (mis)used like this. Please write a dedicated function or just use the GenericHelpers.replaceVars for the dynamic path segments directly.
maxmarkus
requested review from
bszwarc,
kazydek,
klaudiagrz and
tomekpapiernik
as code owners
There was a problem hiding this comment.
Apart from the comments below, I propose the following refactored I did with the support of @pekura:
diff-proposal.zip. Basically the refactoring is around matchPath, since:
- it was tightly coupled to getNavigationPath method
- it could actually be integrated in getNavigationPath method
I am happy to talk about it further ;)
| node.pathSegment.replace(':', '') | ||
| ] = RoutingHelpers.sanitizeParam(urlPathElement); | ||
| } | ||
| const newNodeNamesInCurrentPath = nodeNamesInCurrentPath.slice(); |
There was a problem hiding this comment.
You can write it in just one line const newNodeNamesInCurrentPath = nodeNamesInCurrentPath.slice().shift(); and avoid the mutation of a variable defined as a constant
There was a problem hiding this comment.
Nope, since shift returns the removed item and I need to return everything except the removed item.
There was a problem hiding this comment.
But a .slice(1) is what I was searching for, thanks for pointing again to that issue.
| .replace(/'/g, ''') | ||
| .replace(/\//g, '/'); | ||
| } | ||
| export const substituteObject = (object, paramMap, paramPrefix = ':') => { |
There was a problem hiding this comment.
I would refactor it to this shorter and cleaner version:
export const substituteObject = (object, paramMap, paramPrefix = ':') => {
return Object.entries(object)
.map(([key, value]) => {
let foundKey = Object.keys(paramMap).find((key2) => (value === paramPrefix + key2));
return [key, foundKey ? paramMap[foundKey] : value];
})
.reduce((acc, [key, value]) => {
return Object.assign(acc, { [key]: value });
}, {});
};
| .replace(/'/g, ''') | ||
| .replace(/\//g, '/'); | ||
| } | ||
| export const substituteObject = (object, paramMap, paramPrefix = ':') => { |
There was a problem hiding this comment.
What about finding a better name for this method? I would call it: "substituteDynamicParamsInObject".
…vent-navigation-tree-mutation
…vent-navigation-tree-mutation # Conflicts: # core/src/App.html # core/src/services/routing.js # core/src/utilities/helpers/iframe-helpers.js
Implementation done as proposed in the ticket.
