-
-
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
Merged
Merged
Changes from 26 commits
Commits
Show all changes
33 commits
Select commit
Hold shift + click to select a range
7bb9ca0
Add page parameters to navigation actions
apedroferreira b1f0489
Non-working bindings for actions + fix circular dependency hell
apedroferreira c351057
where did this even come from
apedroferreira 99bb4a3
Fix types
apedroferreira d8c8c0b
Working page parameter bindings
apedroferreira 2007d59
Fix and add test
apedroferreira 005f5d1
Merge remote-tracking branch 'origin/master' into add-navigation-page…
apedroferreira 31d818a
Merge remote-tracking branch 'origin/master' into add-navigation-page…
apedroferreira 2d7dcdc
Merge remote-tracking branch 'origin/master' into add-navigation-page…
apedroferreira 07c2cee
Merge remote-tracking branch 'origin/master' into add-navigation-page…
apedroferreira 4d738a0
Fixes for rebase + new page file structure
apedroferreira 30c2075
hehe
apedroferreira 8bcf041
Remove unneeded fixture things
apedroferreira 13bcc23
Fix page name
apedroferreira d28474c
Merge remote-tracking branch 'origin/master' into add-navigation-page…
apedroferreira e6d0e88
Improve test for navigation overall + see if CI passes
apedroferreira 80a4dfe
Better test
apedroferreira 8e4fec1
Attempt to get CI to pass again
apedroferreira 1fcd96e
Remove check to try to debug issue in CI
apedroferreira 1e44bbb
Revert "Remove check to try to debug issue in CI"
apedroferreira e68d8e0
Merge remote-tracking branch 'origin/master' into add-navigation-page…
apedroferreira c1fe290
Another attempt to make CI pass
apedroferreira 4185ed8
Try again
apedroferreira f8d176d
Try using different method
apedroferreira e2ce209
Merge remote-tracking branch 'origin/master' into add-navigation-page…
apedroferreira e057975
Use $ref instead of $$ref everywhere (mystery solved?)
apedroferreira c246b3d
Forgot this one
apedroferreira 55cd8ba
Refactor with ignored cycle to see what changed
apedroferreira ef1980a
Revert some more changes
apedroferreira 4e1c55c
fix lint
apedroferreira 6ff80f1
Merge remote-tracking branch 'origin/master' into add-navigation-page…
apedroferreira 3d255de
Document navigation actions
apedroferreira 558648b
Allow navigating to same page
apedroferreira File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
48 changes: 48 additions & 0 deletions
48
packages/toolpad-app/src/toolpad/AppEditor/BindingEditor/ActionEditor.tsx
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,48 @@ | ||
import { TabContext, TabList } from '@mui/lab'; | ||
import { Box, Tab } from '@mui/material'; | ||
import * as React from 'react'; | ||
import { BindableAttrValue } from '@mui/toolpad-core'; | ||
import { NavigationActionEditor } from './NavigationActionEditor'; | ||
import TabPanel from '../../../components/TabPanel'; | ||
import { Maybe, WithControlledProp } from '../../../utils/types'; | ||
import { BindableType, JsExpressionActionEditor } from '.'; | ||
|
||
function getActionTab(value: Maybe<BindableAttrValue<any>>) { | ||
return value?.type || 'jsExpressionAction'; | ||
} | ||
|
||
export interface ActionEditorProps extends WithControlledProp<BindableAttrValue<any> | null> {} | ||
|
||
export function ActionEditor({ value, onChange }: ActionEditorProps) { | ||
const [activeTab, setActiveTab] = React.useState<BindableType>(getActionTab(value)); | ||
React.useEffect(() => setActiveTab(getActionTab(value)), [value]); | ||
|
||
const handleTabChange = (event: React.SyntheticEvent, newValue: BindableType) => { | ||
setActiveTab(newValue); | ||
}; | ||
|
||
return ( | ||
<Box> | ||
<TabContext value={activeTab}> | ||
<Box sx={{ borderBottom: 1, borderColor: 'divider' }}> | ||
<TabList onChange={handleTabChange} aria-label="Choose action kind "> | ||
<Tab label="JS expression" value="jsExpressionAction" /> | ||
<Tab label="Navigation" value="navigationAction" /> | ||
</TabList> | ||
</Box> | ||
<TabPanel value="jsExpressionAction" disableGutters> | ||
<JsExpressionActionEditor | ||
value={value?.type === 'jsExpressionAction' ? value : null} | ||
onChange={onChange} | ||
/> | ||
</TabPanel> | ||
<TabPanel value="navigationAction" disableGutters> | ||
<NavigationActionEditor | ||
value={value?.type === 'navigationAction' ? value : null} | ||
onChange={onChange} | ||
/> | ||
</TabPanel> | ||
</TabContext> | ||
</Box> | ||
); | ||
} |
112 changes: 112 additions & 0 deletions
112
packages/toolpad-app/src/toolpad/AppEditor/BindingEditor/BindingEditorDialog.tsx
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,112 @@ | ||
import { Button, Dialog, DialogActions, DialogContent, DialogTitle } from '@mui/material'; | ||
import * as React from 'react'; | ||
import { BindableAttrValue, PropValueType } from '@mui/toolpad-core'; | ||
|
||
import { tryFormatExpression } from '../../../utils/prettier'; | ||
import useShortcut from '../../../utils/useShortcut'; | ||
import useUnsavedChangesConfirm from '../../hooks/useUnsavedChangesConfirm'; | ||
import { WithControlledProp } from '../../../utils/types'; | ||
import { JsBindingEditor, useBindingEditorContext } from '.'; | ||
|
||
export interface BindingEditorDialogProps<V> | ||
extends WithControlledProp<BindableAttrValue<V> | null> { | ||
open: boolean; | ||
onClose: () => void; | ||
renderPropBindingEditor?: ( | ||
propType: PropValueType, | ||
controlProps: WithControlledProp<BindableAttrValue<V> | null>, | ||
) => JSX.Element | null; | ||
} | ||
|
||
export function BindingEditorDialog<V>({ | ||
value, | ||
onChange, | ||
open, | ||
onClose, | ||
renderPropBindingEditor, | ||
}: BindingEditorDialogProps<V>) { | ||
const { propType, label } = useBindingEditorContext(); | ||
|
||
const [input, setInput] = React.useState(value); | ||
React.useEffect(() => { | ||
if (open) { | ||
setInput(value); | ||
} | ||
}, [open, value]); | ||
|
||
const committedInput = React.useRef<BindableAttrValue<V> | null>(input); | ||
|
||
const handleSave = React.useCallback(() => { | ||
let newValue = input; | ||
|
||
if (input?.type === 'jsExpression') { | ||
newValue = { | ||
...input, | ||
value: tryFormatExpression(input.value), | ||
}; | ||
} | ||
|
||
committedInput.current = newValue; | ||
onChange(newValue); | ||
}, [onChange, input]); | ||
|
||
const hasUnsavedChanges = input | ||
? input.type !== committedInput.current?.type || input.value !== committedInput.current?.value | ||
: false; | ||
|
||
const { handleCloseWithUnsavedChanges } = useUnsavedChangesConfirm({ | ||
hasUnsavedChanges, | ||
onClose, | ||
}); | ||
|
||
const handleCommit = React.useCallback(() => { | ||
handleSave(); | ||
onClose(); | ||
}, [onClose, handleSave]); | ||
|
||
const handleRemove = React.useCallback(() => { | ||
onChange(null); | ||
onClose(); | ||
}, [onClose, onChange]); | ||
|
||
useShortcut({ key: 's', metaKey: true, disabled: !open }, handleSave); | ||
|
||
const propBindingEditor = | ||
renderPropBindingEditor && | ||
propType && | ||
renderPropBindingEditor(propType, { | ||
value: input, | ||
onChange: (newValue) => setInput(newValue), | ||
}); | ||
|
||
return ( | ||
<Dialog | ||
onClose={handleCloseWithUnsavedChanges} | ||
open={open} | ||
fullWidth | ||
scroll="body" | ||
maxWidth="lg" | ||
> | ||
<DialogTitle>Bind property "{label}"</DialogTitle> | ||
<DialogContent> | ||
{propBindingEditor || ( | ||
<JsBindingEditor | ||
value={input?.type === 'jsExpression' ? input : null} | ||
onChange={(newValue) => setInput(newValue)} | ||
/> | ||
)} | ||
</DialogContent> | ||
<DialogActions> | ||
<Button color="inherit" variant="text" onClick={onClose}> | ||
{hasUnsavedChanges ? 'Cancel' : 'Close'} | ||
</Button> | ||
<Button color="inherit" disabled={!value} onClick={handleRemove}> | ||
Remove binding | ||
</Button> | ||
<Button disabled={!hasUnsavedChanges} color="primary" onClick={handleCommit}> | ||
Update binding | ||
</Button> | ||
</DialogActions> | ||
</Dialog> | ||
); | ||
} |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
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.
Choose a reason for hiding this comment
The 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 comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The 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.
Otherwise, if it works, it works, and this can be merged
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 forgot this comment... will take a second look and add changes if it makes sense.