-
-
Notifications
You must be signed in to change notification settings - Fork 274
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
Add page parameters to navigation actions #1876
Changes from all commits
7bb9ca0
b1f0489
c351057
99bb4a3
d8c8c0b
2007d59
005f5d1
31d818a
2d7dcdc
07c2cee
4d738a0
30c2075
8bcf041
13bcc23
d28474c
e6d0e88
80a4dfe
8e4fec1
1fcd96e
1e44bbb
e68d8e0
c1fe290
4185ed8
f8d176d
e2ce209
e057975
c246b3d
55cd8ba
ef1980a
4e1c55c
6ff80f1
3d255de
558648b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -1114,15 +1114,15 @@ export function ref(nodeId: NodeId): NodeReference; | |
export function ref(nodeId: null | undefined): null; | ||
export function ref(nodeId: Maybe<NodeId>): NodeReference | null; | ||
export function ref(nodeId: Maybe<NodeId>): NodeReference | null { | ||
return nodeId ? { $$ref: nodeId } : null; | ||
return nodeId ? { $ref: nodeId } : null; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. When we inject the initial DOM state in the HTML in the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We currently have two formats for bindings/actions/... One legacy one in the appDom, one new one from the new file system format. Over time we will move the whole application over to the second format There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ok! How do you suggest fixing the navigation actions for now? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Since you're changing the existing format, my suggestion would be to change it to align it close to the what's in the file schema. The more we can reduce the amount of conversion needed from schema to internal representation, the better. With the north star goal to get rid of the conversion completely. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I forgot this comment... will take a second look and add changes if it makes sense. |
||
} | ||
|
||
export function deref(nodeRef: NodeReference): NodeId; | ||
export function deref(nodeRef: null | undefined): null; | ||
export function deref(nodeRef: Maybe<NodeReference>): NodeId | null; | ||
export function deref(nodeRef: Maybe<NodeReference>): NodeId | null { | ||
if (nodeRef) { | ||
return nodeRef.$$ref; | ||
return nodeRef.$ref; | ||
} | ||
return null; | ||
} | ||
|
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 is to be done here?
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.
We should document the feature of being able to set page parameters separately from navigation actions in my opinion, and then we could link to it from here.
But I wasn't sure at the time in which section the page parameters should be documented, so I added this todo. I can make sure we do this soon enough as we're working on the docs.