Add page parameters to navigation actions #1876
Conversation
github-actions
bot
added
the
PR: out-of-date
github-actions
bot
removed
the
PR: out-of-date
| @@ -385,6 +385,7 @@ function mergeThemeIntoAppDom(dom: appDom.AppDom, themeFile: Theme): appDom.AppD | |||
| } | |||
|
|
|||
| function toBindable<V>(value: V | { $$jsExpression: string }): BindableAttrValue<V> { | |||
| console.log('fuck'); | |||
apedroferreira
force-pushed
the
add-navigation-page-params
branch
from
467a7d5
to
8e4fec1
Compare
apedroferreira
force-pushed
the
add-navigation-page-params
branch
2 times, most recently
from
1670609
to
42d8617
Compare
apedroferreira
force-pushed
the
add-navigation-page-params
branch
from
42d8617
to
e057975
Compare
| @@ -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.
When we inject the initial DOM state in the HTML in the <script> tags all the $$ref properties are being changed to $ref now with the Vite runtime... Not sure exactly why.
So all navigation actions were broken.
Can we just use $ref for these properties? It shouldn't even need a migration, right? I only saw it being used in a few old connections besides navigation actions.
There was a problem hiding this comment.
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.
Ok! How do you suggest fixing the navigation actions for now? appDom.deRef tries to get $$ref but the local DOM has the page as $ref.
Are the changes in this PR about that ok or do you suggest an alternative?
There was a problem hiding this comment.
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.
Otherwise, if it works, it works, and this can be merged
There was a problem hiding this comment.
I forgot this comment... will take a second look and add changes if it makes sense.
apedroferreira
force-pushed
the
add-navigation-page-params
branch
from
ea337fd
to
ef1980a
Compare
|
I've also allowed navigating to the current page to close #1579 too. |
| <img src="/static/toolpad/docs/data-binding/bind-12.png" alt="Binding editor for navigation actions" width="800" /> | ||
|
|
||
| <!--- | ||
| @TODO: Link "any query string parameters that the destination page supports" to page parameters documentation |
There was a problem hiding this comment.
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.
Closes #1800
Closes #1579
Allow setting destination page parameters in navigation actions, including binding them to the current page state.
Also add documentation about navigation actions.
Screen.Recording.2023-04-27.at.22.30.15.mov